Fix Time.new parsing (time/new_tags.txt)#8890
Merged
enebo merged 1 commit intojruby:masterfrom Jul 5, 2025
Merged
Conversation
The idea was to help towards this jruby#8736 but most of the fixes here are specifically around parsing the time string, not timezone related. @enebo I noticed you did recent changes around Time.new parsing as well, in case you want to review. I'll add some comments in the PR to highlight which changes fix which parts, but here is the TLDR: - Time.new parsing should ignore "whitespace" chars, not only space chars - precision keyword argument should only be used by Time.new parsing and ignored otherwise (see Ruby docs), and it should default to 9 - RubyTimeParser.parseInt now returns nil if no digits (so missing year can be detected) and only to a set number of digits (for subsec precision) - correctly convert subsec to microseconds in RubyTimeParser when more than 9 digits
PChambino
commented
Jul 4, 2025
| fails(not implemented, jruby/jruby#6161):Time.new with a timezone argument subject's class implements .find_timezone method calls .find_timezone to build a time object at loading marshaled data | ||
| fails(only during full spec run):Time.new with a utc_offset argument raises ArgumentError if the String argument is not of the form (+|-)HH:MM | ||
| fails:Time.new with a timezone argument returned value by #utc_to_local and #local_to_utc methods cannot have arbitrary #utc_offset if it is an instance of Time | ||
| fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument :in keyword argument allows omitting minor arguments |
Contributor
Author
There was a problem hiding this comment.
This was already passing, I think I might have fixed it in my previous PR: #8858
But I wasn't aware of these tag files.
PChambino
commented
Jul 4, 2025
| fails:Time.new with a timezone argument returned value by #utc_to_local and #local_to_utc methods cannot have arbitrary #utc_offset if it is an instance of Time | ||
| fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument :in keyword argument allows omitting minor arguments | ||
| fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument Time.new with a String argument accepts precision keyword argument and truncates specified digits of sub-second part | ||
| fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument Time.new with a String argument converts precision keyword argument into Integer if is not nil |
Contributor
Author
There was a problem hiding this comment.
Removing the type error in RubyTime basically fixed this. We already call toLong in the RubyTimeParser.
I also noticed that the precision keyword argument is only supposed to be used when parsing a ruby time string and ignore otherwise, and it should default to 9.
PChambino
commented
Jul 4, 2025
| fails(only during full spec run):Time.new with a utc_offset argument raises ArgumentError if the String argument is not of the form (+|-)HH:MM | ||
| fails:Time.new with a timezone argument returned value by #utc_to_local and #local_to_utc methods cannot have arbitrary #utc_offset if it is an instance of Time | ||
| fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument :in keyword argument allows omitting minor arguments | ||
| fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument Time.new with a String argument accepts precision keyword argument and truncates specified digits of sub-second part |
Contributor
Author
There was a problem hiding this comment.
RubyTime only has precision up to 9 (nanoseconds) so it is not possible to remove this tag, but the microseconds conversion was wrong ( + 3 vs - 3) when there were more than 9 digits. So it now truncates correctly.
PChambino
commented
Jul 4, 2025
| fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument Time.new with a String argument converts precision keyword argument into Integer if is not nil | ||
| fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument Time.new with a String argument raises ArgumentError if date/time parts values are not valid | ||
| fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument Time.new with a String argument raises ArgumentError if utc offset parts are not valid | ||
| fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument Time.new with a String argument raises ArgumentError if string doesn't start with year |
Contributor
Author
There was a problem hiding this comment.
Changed the RubyTimeParser.parseInt to return nil if no digits, so we can raise the correct error. Also, this error needs to have the str.inspect() in the error message instead of the plain string.
PChambino
commented
Jul 4, 2025
Comment on lines
-18
to
-19
| fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument Time.new with a String argument raises ArgumentError when there are leading space characters | ||
| fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument Time.new with a String argument raises ArgumentError when there are trailing whitespaces |
Contributor
Author
There was a problem hiding this comment.
These two were just about ignoring more "whitespace" chars, not just spaces.
PChambino
commented
Jul 4, 2025
| fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument Time.new with a String argument raises ArgumentError if string doesn't start with year | ||
| fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument Time.new with a String argument raises ArgumentError when there are leading space characters | ||
| fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument Time.new with a String argument raises ArgumentError when there are trailing whitespaces | ||
| fails:Time.new with a timezone argument Time.new with a String argument returns Time with correct subseconds when given seconds fraction is longer than 9 digits |
Contributor
Author
There was a problem hiding this comment.
This was fixed by adding support for "precision" in RubyTimeParser.parseInt.
PChambino
commented
Jul 4, 2025
| fails:Time.new with a utc_offset argument returns a Time with a UTC offset of the specified number of Rational seconds | ||
| fails:Time.new with a utc_offset argument raises ArgumentError if the String argument is not in an ASCII-compatible encoding | ||
| fails:Time.new with a utc_offset argument with an argument that responds to #to_r coerces using #to_r | ||
| fails:Time.new with a utc_offset argument raises ArgumentError if the month is greater than 12 |
Contributor
Author
There was a problem hiding this comment.
This was already passing, so I just removed it.
Member
|
@PChambino cool. The -3 vs +3 makes sense and explains a confusion I had when working on the other recent PR I did 😄. I am mildly confused about the already passing specs since I thought I had did a mspec-tag to untag passing things but I must have done that manually? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The idea was to help towards this #8736 but most of the fixes here are specifically around parsing the time string, not timezone related.
@enebo I noticed you did recent changes around Time.new parsing as well, in case you want to review.
I'll add some comments in the PR to highlight which changes fix which parts, but here is the TLDR:
Time.new parsing should ignore "whitespace" chars, not only space chars
precision keyword argument should only be used by Time.new parsing and ignored otherwise (see Ruby docs), and it should default to 9
RubyTimeParser.parseInt now returns nil if no digits (so missing year can be detected) and only to a set number of digits (for subsec precision)
correctly convert subsec to microseconds in RubyTimeParser when more than 9 digits