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 Rubinius 2.0 #7

Closed
wants to merge 1 commit into from
Closed

Fix for Rubinius 2.0 #7

wants to merge 1 commit into from

Conversation

wulftone
Copy link

I ran into a problem where this gem wasn't working when I wanted to switch to Rubinius, so I tracked this down and fixed it.

Since this relies on monkeypatching a function (_parse), that a user wouldn't normally call, and internally Rubinius uses a different arity on _parse, I think this patch is probably a good idea. : ) What do you think?

@jeremyevans
Copy link
Owner

Date._parse is a public method, and users would usually call it if they want to get a hash of values as opposed to a Date instance (I do this in Sequel to check if the date string contains an offset). I think adding additional optional arguments changes the API, and I'm not sure that's something they want to be doing.

Have you confirmed with the Rubinius developers that this is not going to change? I'm guessing they are having Date.parse call Date._parse with this flag set, instead of having both methods call a separate internal method (which would be the safer approach).

Assuming this behavior is not going to change, a more robust approach would be to check the arity of _parse_without_american_date instead of assuming 3 arguments on rbx.

@wulftone
Copy link
Author

Yeah, that makes sense. I don't know if they're aware of it... I'll see what they have to say.

@wulftone
Copy link
Author

wulftone commented Apr 2, 2013

They appear to be doing something. I'll just close this now. rubinius/rubinius#2241

@wulftone wulftone closed this Apr 2, 2013
@wulftone wulftone deleted the rubinius branch April 2, 2013 20:34
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.

None yet

2 participants