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

Match CEL and Go duration literal parsing, while preserving the full range of values #38

Merged
merged 9 commits into from
Jul 29, 2024

Conversation

Alfus
Copy link
Contributor

@Alfus Alfus commented Jul 28, 2024

Matches the impl in time.ParseDuration, except uses a big.Int to support the full range of protobuf Duration values.

@Alfus Alfus requested a review from rodaine July 28, 2024 18:01
decode.go Outdated
var nanosPerSecond = new(big.Int).SetUint64(uint64(time.Second / time.Nanosecond))

var nanosMap = map[string]*big.Int{
"ns": new(big.Int).SetUint64(1),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be consistent with the rest of the lookups:

Suggested change
"ns": new(big.Int).SetUint64(1),
"ns": new(big.Int).SetUint64(time.Nanosceond),

Copy link
Contributor Author

@Alfus Alfus Jul 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the map to nanoseconds, it converts all the other units to nanos. It just happens to be that time.Nanoseconds is 1, to be consistent this would be:

new(big.Int).SetUint64(uint64(time.Nanosecond / time.Nanosecond)),

but the linter doesn't like that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, but the random magic 1 takes away from the implied documentation. What does the linter complain about?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also suggest dropping the / time.Nanosecond as well, since it's also redundant if you know about the type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, but this conde isn't supposed to know that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added docs to clarify what these values represent (which is the number of nanos in each unit)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go's not going to change the unit value to pico's until at least Go2 XD

decode.go Show resolved Hide resolved
decode_test.go Outdated Show resolved Hide resolved
@Alfus Alfus merged commit 93c0894 into main Jul 29, 2024
5 checks passed
@Alfus Alfus deleted the alfus/duration branch July 29, 2024 18:27
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.

2 participants