Skip to content
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

Fix for #3 and #4 #5

Merged
merged 1 commit into from Jul 22, 2013
Merged

Fix for #3 and #4 #5

merged 1 commit into from Jul 22, 2013

Conversation

ghost
Copy link

@ghost ghost commented Jul 22, 2013

Fix for #3 and #4. I've included a regression test for #3. You can verify that this test reproduces #3 by temporarily changing line 58 of NonBlockingStatsDClient from:

NumberFormat numberFormatter = NumberFormat.getInstance(Locale.US);

to

NumberFormat numberFormatter = NumberFormat.getInstance();

which is consistent with the previous version. After making this change the regression test will fail. Could you please make a new release ASAP after this pull request has been merged, because currently this library is not usable in any locale that uses something other than a period as the decimal separator (Germany, France, etc.).

@ghost
Copy link
Author

ghost commented Jul 22, 2013

Sorry about the weird commit message

@charles-dyfis-net
Copy link

No problem (and I very much appreciate the contribution). That said, would you mind editing your commit to fix the message and doing a force-push to the branch this pull request is attached to?

@ghost
Copy link
Author

ghost commented Jul 22, 2013

I've fixed the commit message. BTW kaufda is also a GitHub account of mine, there's some weird interference happening between these accounts that caused the bad commit message.

Do you think it might be possible for you to release this new version soon (within the next hour or so)? If not, I'll deploy the fixed version to my local repository, while I'm waiting for an official release that includes these fixes.

@charles-dyfis-net
Copy link

It won't make it to Maven Central within the hour, but if you don't mind pulling directly from Sonatype, I can do that.

charles-dyfis-net added a commit that referenced this pull request Jul 22, 2013
Fixes for #3 and #4 (locale dependence and thread-safety issues). Many thanks to @domurtag / @kaufda.
@charles-dyfis-net charles-dyfis-net merged commit 6473169 into indeedeng:master Jul 22, 2013
@ghost
Copy link
Author

ghost commented Jul 22, 2013

Thanks for merging this so quickly. I'm not exactly sure what you mean by

If you don't mind pulling directly from Sonatype, I can do that.

Is this some other repo that I can download it from? Could you let me know what the new version number will be, so that I can check when it's available in Maven central?

@charles-dyfis-net
Copy link

We use the Sonatype OSSRH repository for publishing to central; things only get picked up from there on a cron job that runs every two hours.

It's available as 2.0.8, but until it syncs to Central, you'll need to have the following repository available as a source: https://oss.sonatype.org/content/repositories/releases/

@ghost
Copy link
Author

ghost commented Jul 22, 2013

I've added the Sonatype repo and it seems to be working, thanks for your help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants