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
Implement pydantic models as dataclasses #210
Implement pydantic models as dataclasses #210
Changes from 1 commit
0461d20
04ac60e
0fd50e8
ea15b8e
7b1471b
b730692
6f7c1ba
1b27954
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.
So order of fields in the kerchunk references matters now? Did it matter before? This should be fine but it's also the sort of thing that kerchunk might not be consistent about...
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.
No, it still didn't matter before and still doesn't, as far as using Kerhcunk goes :) This reason this matters for the test is because we're doing string comparison on the
"a/.zarray"
field. We could also parse that JSON into a dict before doing the comparision, and I wouldn't have needed to update the expected.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.
Do we want the list of fields that can be replaced here in the signature?
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.
As opposed to letting users call dataclasses.replace? I think
ZArray
shouldn't have fields that aren't replaceable, are there any fields like that?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 think if
dataclasses.replace
exists already then explicitly writing the list of fields isn't a good enough reason to override the default method.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.
Oh no wait
dataclasses.replace
is a module-level function, not a method on the dataclass type. In that case I change my mind - we should list out the arguments and calldataclasses.replace
from within theZArray.replace
method.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.
Just to be clear, the behavior here (and on
main
) is to return a new instance ofZArray
with the same values as the old instance, but new values specified in the keyword arguments.Sounds good. I think we'll be able to do that (we'll need to remember to keep the two signatures in step though).