-
Notifications
You must be signed in to change notification settings - Fork 444
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added group_separator feature in number formatting #726
Conversation
0e89a0b
to
5150e7b
Compare
Codecov Report
@@ Coverage Diff @@
## master #726 +/- ##
=======================================
Coverage 90.97% 90.97%
=======================================
Files 24 24
Lines 4176 4179 +3
=======================================
+ Hits 3799 3802 +3
Misses 377 377
Continue to review full report at Codecov.
|
@akx @kdeldycke please have a look. |
Looks nice @Abdullahjavednesar! Can you add some test to demonstrates the use of explicit It might also be great to have the docstrings updated so we can have that new option documented for end-users. |
Hi @kdeldycke, thanks for the review.
Sure, will add more tests to cover those too.
Yes, makes sense, I'll update the docstrings. |
4e745b4
to
f6e4b2d
Compare
f6e4b2d
to
f25cee0
Compare
Hi @kdeldycke @akx please have a look. If looks good, can this be merged? |
LGTM! Thanks @Abdullahjavednesar for your contribution! I'm no official maintainer though, I just added a couple of features in the same spirit as yours, and as a fellow contributor I find it great! :) Now we need a review and eventual merge decision from an official maintainer. |
Thanks a lot @kdeldycke for your reviews, and those good words. Indeed Open source is a great platform to work and learn collaboratively :) @akx a request if you could please review this :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine by me, though I'd like to know what the actual use case for this is :)
Thanks @akx for merging :) We came across this use case where we had to export files with Amount column and amount should just have a fraction symbol and not the group symbol. I believe this would be the cleanest approach so I've added this feature to all applicable functions. Thanks again @kdeldycke @akx :) |
Added a new group_separator feature in number formatting. group_separator is a Boolean parameter default set as
True
, whenFalse
, it simply means the output should be free of group separator.Example:
Default behaviour remains unchanged.
Example: