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.
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
Duration/Memory Convenience Click Types #448
Duration/Memory Convenience Click Types #448
Changes from 4 commits
a333f6b
f2edc27
33ad611
bbfa1c2
857f9e6
dcb899b
4071d37
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is forbidding "years" about that being conceptually wrong or simply a duration we don't want to contemplate? seems as a generic thing, its appropriate for the tool to potentially support years, so a bit weird to explicitly test that it doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two issues with years: 1) years do not have a constant length (2024 was 366 days but most years are 365 days), and 2) python's
timedelta
class which doesn't have ayears
argument: https://docs.python.org/3/library/datetime.html#datetime.timedelta.I could however see an argument for adding micro/milliseconds on conceptual ground if you would like? I just excluded those since it was too short for most practical use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this is a duration we explicitly don't want to contemplate (primarily because it's an ambiguous unit for our case) - that's fine! It does feel worth noting somewhere that we don't allow this unit of time - like maybe in the method documentation? But I don't think we need, say, a special error message for years.
re ms - I think it's fine to not support it at this time. I think you're saying this would be one that we'd be willing to contemplate (i.e. we aren't testing to ensure its exclusion), but that we haven't bothered to yet support (because we don't think its practically useful).