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

Inconsistent behavior of getMilliseconds with cel-spec #453

Open
1 of 3 tasks
TristonianJones opened this issue Sep 21, 2021 · 3 comments
Open
1 of 3 tasks

Inconsistent behavior of getMilliseconds with cel-spec #453

TristonianJones opened this issue Sep 21, 2021 · 3 comments

Comments

@TristonianJones
Copy link
Collaborator

Describe the bug
The getMilliseconds behavior matches user expectations but does not match cel-spec. Eventually this function will be deprecated and the spec revised in favor of a function that matches user expectations.

To Reproduce
Check which components this affects:

  • parser
  • checker
  • interpreter

Sample expression and input that reproduces the issue:

// Returns 23ms rather than converting the value to 1023.
duration("1s23ms").getMilliseconds()

Test setup:
See google/cel-spec#206 for more context on how to test this functionality.

Expected behavior
The function returns the milliseconds component of the duration.

@kunaltawatia
Copy link

I looked into this issue and figured that duration.go#overloads.TimeGetMilliseconds and duration_test.go#TestDurationGetMilliseconds shall require the changes.
If this issue is not assigned yet, can I take a stab at this?
I believe this would be an excellent opportunity for me to start contributing here.

@TristonianJones
Copy link
Collaborator Author

@kunaltawatia The existing behavior should be moved to a new function, but we have yet to finalize the name of the function. I'll follow up here once the new function name is identified and if you're still interested, you are welcome to work on it.

@kunaltawatia
Copy link

Sure, that'd not be a problem.
Curious, Is the change of name required to avoid regression, or are there any other reasons?

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

No branches or pull requests

2 participants