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

Bunch of changes and refinements #69

Merged
merged 21 commits into from
Dec 11, 2012
Merged

Bunch of changes and refinements #69

merged 21 commits into from
Dec 11, 2012

Conversation

rwz
Copy link
Member

@rwz rwz commented Dec 11, 2012

I've just decided to put everything into a single pull-request.

I can split this into multiple pull-requests if you wish.

@rwz
Copy link
Member Author

rwz commented Dec 11, 2012

Ah, also.

I couldn't run tests on macruby. So can't be sure I didn't screw up the nsjsonserialization. Need to check this.

@sferik
Copy link
Member

sferik commented Dec 11, 2012

These changes look good…

Would you mind rebasing from master so this will merge cleanly? I suspect you'll hit a few conflicts, since I recently touched some of the same code to get JRuby specs running/passing on Travis CI.

@rwz
Copy link
Member Author

rwz commented Dec 11, 2012

Yep, doing that at the moment :)

@sferik
Copy link
Member

sferik commented Dec 11, 2012

Looks like there are some date format issues on Ruby 1.8.7.

@rwz
Copy link
Member Author

rwz commented Dec 11, 2012

@sferik what do you think, shoud I just remove this spec or make it respect weird 1.8 time format?

@rwz
Copy link
Member Author

rwz commented Dec 11, 2012

Added a case for 1.8 time format.

@sferik
Copy link
Member

sferik commented Dec 11, 2012

Hmmm. Do you think it's okay to have different results in Ruby 1.8 and Ruby 1.9? I think this could be problematic.

@rwz
Copy link
Member Author

rwz commented Dec 11, 2012

I'm not sure if it's ok or not. At the same time I don't see any easy way to make it use consistent format.

Also, pretty much all JSON gems code Time that way, so, folks on 1.8 are probably aware of that and have all codes behave accordingly.

@sferik
Copy link
Member

sferik commented Dec 11, 2012

It looks like your patch fails on JRuby. It's not obvious to me what the issue is. Can you please investigate? I don't want to merge in code that breaks the build. It may be helpful to ask in the #jruby IRC channel on Freenode.

@rwz
Copy link
Member Author

rwz commented Dec 11, 2012

Will do.

@rwz
Copy link
Member Author

rwz commented Dec 11, 2012

The problems is Time test I've introduced here: rwz@d692b8e

If you comment it, jruby passes the specs just fine. Weird.

@sferik
Copy link
Member

sferik commented Dec 11, 2012

Strange indeed. I'd recommend asking the folks in #jruby about that. I think they could point you in the right direction. Or at least file a bug.

@rwz
Copy link
Member Author

rwz commented Dec 11, 2012

a comment from @headius

headius: rwz: I'd say to file an issue and turn them off for the moment
headius: we're losing our verve to maintain  C ext support

@rwz
Copy link
Member Author

rwz commented Dec 11, 2012

Should be all-green now. Travis just decided to reboot servers during latest build :(

@sferik
Copy link
Member

sferik commented Dec 11, 2012

I had a feeling he might say that. 😦

@sferik
Copy link
Member

sferik commented Dec 11, 2012

This looks beautiful! Thanks again for your work on this. 🍻

sferik added a commit that referenced this pull request Dec 11, 2012
Bunch of changes and refinements
@sferik sferik merged commit b46f137 into intridea:master Dec 11, 2012
@jeremygpeterson
Copy link

These changes will help me resolve karmi/retire#510 in the tire gem. Any idea when this might be released?

@sferik
Copy link
Member

sferik commented Dec 29, 2012

I'd like to get #71 merged in before I ship 1.6.0.

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.

4 participants