chrono icon indicating copy to clipboard operation
chrono copied to clipboard

Allow nanoseconds unix epoch, and numeric literal after year format

Open titaneric opened this issue 10 months ago • 7 comments

This PR aims to resolve issue #560 and #1399 based on PR #561, and the multiple issues from vector such as https://github.com/vectordotdev/vrl/issues/117, https://github.com/vectordotdev/vrl/issues/790 Since there is no any respones from the author of #561, I would like to continue his/her work.

titaneric avatar Jan 11 '25 09:01 titaneric

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 91.07%. Comparing base (97f758f) to head (1922a29). Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1648      +/-   ##
==========================================
+ Coverage   90.87%   91.07%   +0.19%     
==========================================
  Files          37       37              
  Lines       17143    17459     +316     
==========================================
+ Hits        15579    15901     +322     
+ Misses       1564     1558       -6     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jan 11 '25 09:01 codecov[bot]

@meiamsome , @pitdicker , @djc, @jszwedko , sorry to ping you here since you have discussed the topic in #561 , but I would like to hear any comments to my PR.

titaneric avatar Jan 18 '25 06:01 titaneric

@meiamsome , @pitdicker , @djc, @jszwedko , sorry to ping you here since you have discussed the topic in #561 , but I would like to hear any comments to my PR.

I'll defer comments on the implementation since I'm not super familiar with chrono internals, but the tests look good to me. Thanks for attempting to fix this!

jszwedko avatar Jan 21 '25 20:01 jszwedko

I don't like these changes much. They seem to be very focused on just passing the added tests and don't even try to clean up the surrounding code to understand that the underlying motivation is clearly addressed.

djc avatar Jan 22 '25 09:01 djc

Present implementation heavily depends on previous attempted PR and one of the comment. I substract the next possible numerical literal to count the actual token size for either year or timestamp, that's two phase calculation but would only happen in the special case.

Are you saying that I need to do more work to the parse_internal function? Trying to calculate the token size with one-phase only?

titaneric avatar Jan 22 '25 10:01 titaneric

Are you saying that I need to do more work to the parse_internal function? Trying to calculate the token size with one-phase only?

I am saying that I as the maintainer don't find the presented changes an improvement in clarity to already pretty convoluted code. I won't pretend to know exactly how to solve it, which would take me a bunch more time which I'm not really motivated to spend unless some funding is available.

djc avatar Jan 24 '25 13:01 djc

I have refactored the implementation a little bit, hopefully it could make the code more readable and concise. Please review it whenever you are available.

titaneric avatar Jan 27 '25 17:01 titaneric