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

Resolve #9 - Different Handling for Negative "-" and Positive "+" Signs #11

Merged
merged 1 commit into from Mar 13, 2018

Conversation

Projects
None yet
3 participants
@hyyan
Contributor

hyyan commented Mar 13, 2018

No description provided.

@Mottie

This comment has been minimized.

Owner

Mottie commented Mar 13, 2018

Hi @hyyan!

I agree with what you're saying. The behavior you describe does make more sense.

This library could use a rewrite, but I'll go ahead and merge this change if you would please fix the failing test.

@Mottie Mottie added the enhancement label Mar 13, 2018

@hyyan

This comment has been minimized.

Contributor

hyyan commented Mar 13, 2018

@Mottie that's great, the tests are passing , but Travis is falling for a different reason, you might want to check that first, but you can try the PR locally and run the tests

@Mottie

This comment has been minimized.

Owner

Mottie commented Mar 13, 2018

Yeah, I see the tests are passing locally... I think it'll be better if I switch the tests to ava.

@Mottie

This comment has been minimized.

Owner

Mottie commented Mar 13, 2018

Cool, I'll go ahead and merge this... I won't update npm until I get stuff cleaned up.

@Mottie Mottie merged commit e8d2418 into Mottie:master Mar 13, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@Mottie

This comment has been minimized.

Owner

Mottie commented Sep 6, 2018

I've published, then unpublished the changes made by this PR because it was a breaking change (see #17)... I think it would be better to add this behavior behind an option. I just can't figure out what to name it.

I think it's weird to completely ignore the negative value here:

format("-#,##0.######", 5000.123456789); //=> 5,000.123457

Was this intended? What do you think the option should be named? Maybe enforceMaskSign?

@rotsee

This comment has been minimized.

rotsee commented Oct 25, 2018

I would like to see this PR somehow re-implemented.
I was trying to use this lib to parse subpatterns drawn from CLDR, but stumbled on explicit plus signs not working as expected!

edit: Or maybe you could publish 1.1.12 to npm?

edit2: I realize now there is a whole discussion going on around this. I'll wait and see, then! :)

@Mottie

This comment has been minimized.

Owner

Mottie commented Oct 26, 2018

I cleaned up the code, added an enforceMaskSign option and published v2.0.5 to npm.

The "complete" rewrite to implement the other open enhancement requests hasn't been finished yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment