-
Notifications
You must be signed in to change notification settings - Fork 185
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
Support Time.new with string timestamp argument and error when invalid #3702
Conversation
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA). To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, it look great and simple.
I left some comments to improve it further.
9b52a02
to
4023a25
Compare
Thank you for signing the OCA. |
if /\A (?<year>\d{4,5}) | ||
(?: | ||
- (?<month>\d{2}) | ||
- (?<mday> \d{2}) | ||
[ T] (?<hour> \d{2}) | ||
: (?<min> \d{2}) | ||
: (?<sec> \d{2}) | ||
(?:\. (?<usec> \d+) )? | ||
\s* (?<offset>\S+)? | ||
)?\z/x =~ str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful :)
Co-authored-by: Kevin Menard <[email protected]>
4023a25
to
fb8c1af
Compare
closes #3693
Use a regexp to quickly detect if a timestamp string is valid.
If it isn't raise an ArgumentError for consistency with CRuby 3.2+.
I added a few new specs for behavior I noticed that I didn't see in the existing specs.
It's not that difficult to match the existing error messages, but I'm not sure how valuable they are.
Here are some examples of the differences:
As discussed, this relaxes the error message specs to keep the code simple.
This removes the "fails" tags for the "Time.new with a String argument" specs except for the three
precision
related ones which TruffleRuby isn't doing anything with yet.