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
Add function to parse quantity #37
Add function to parse quantity #37
Changes from all commits
f912a05
da59728
18d9f85
4d01b7b
4fdd781
7eb03fe
0e24788
fd9c9de
20a9647
a496c36
b8185b7
d75b4ab
ace9a2f
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.
I don't see sufficient benefits of supporting dicts. Users can always convert dicts to ResourceRequirements (
ResourceRequirements.from_dict
). I'll suggest to drop this for now, so that we can have a simpler implementation.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.
The dict here is not an alternative representation of
ResourceRequirements
, but ofResourceRequirements.limits
andResourceRequirements.requests
, i.e. one level down.The tests demonstrate this. With the overload we're able to do both:
and
If feasible, we could change ResourceRequirements to use TypedDict
In which case the overload would become
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.
Got it, I was confused by the function description "Compare two resource requirements for numerical equality." I assumed that the only acceptable type is
ResourceRequirements
or the equivalent dict. Maybe we can clarify further?No need to introduce the TypedDict as anyway all the keys are optional and we can have further keys.
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.
Fixed in next commit.