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

Allow nanoseconds unix epoch, and numeric literal after year format #1648

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

titaneric
Copy link

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

Copy link

codecov bot commented Jan 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.92%. Comparing base (97f758f) to head (ced88b2).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1648      +/-   ##
==========================================
+ Coverage   90.87%   90.92%   +0.04%     
==========================================
  Files          37       37              
  Lines       17143    17230      +87     
==========================================
+ Hits        15579    15666      +87     
  Misses       1564     1564              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@titaneric
Copy link
Author

@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.

@jszwedko
Copy link

@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!

@djc
Copy link
Member

djc commented Jan 22, 2025

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.

@titaneric
Copy link
Author

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?

@djc
Copy link
Member

djc commented Jan 24, 2025

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.

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.

3 participants