Custom number formatting #112

Closed
wants to merge 18 commits into from

3 participants

@therazor

These commits implement the original suggestion in jquery/globalize#88.
The format is heavily based on C# number formatting (pretty similar to most other languages).
It doesn't currently support: number scaling, percent, per mille, exponential notation.

grunt.js has been rewritten according to the documentation on https://github.com/cowboy/grunt.

Full test coverage is included in a new file, and the code has been linted.

therazor added some commits May 15, 2012
@mikesherov
jQuery Foundation member

So, a couple of comments:

  1. Don't work out of master. You should create a separate branch to work out of, which you should be rebasing against master rather than merging master into it. This keeps your pull request clean and only includes changes you made and no merges.
  2. this is actually several pull requests rolled into one... There should be a separate pull request to update grunt and lint the code. In fact, you don't even need the grunt portion now that another commit is present that updated it, so please remove it. This is the main reason to keep pulls small and focused. If you have a large pull with multiple goals being accomplished, it becomes hard to unravel if another commit comes in that overrides your work.
  3. I think you did a great job with the test suite changes. It's good to see a solid set of tests to accompany this change.

So in order for this pull request to be considered, it would have to:

  1. be from a branch, not master, that preferably doesn't contain merge commits.
  2. not contain grunt or jshint changes
@therazor

Thank you for your comments Mike!

Making a separated branch is definitely a good idea, will make a new one and request a new pull from there.
While I'd generally agree about "focused" pulls, some of the unrelated parts were required - e.g. qunit had some bugs that prevented the code added from passing the tests.
Will keep them separated though, even if it means that a pull doesn't pass the tests, which seems kinda awkward.

New pull requests inc next week

@therazor therazor closed this Oct 27, 2012
@sampathaya
@therazor

hi @sampathaya, the "author" of this project is the jquery ui team.
You can see some of the ways to contact them on the "getting involved" page.

I recommend using one of the methods listed there as commenting on a totally unrelated, closed pull request is unlikely to be noticed :)

@therazor therazor referenced this pull request Jan 27, 2013
Closed

Custom formatting #146

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