-
Notifications
You must be signed in to change notification settings - Fork 10
[WIP] Serialization of StagingPaths #259
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
Conversation
|
Hello @dwhswenson! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2023-12-22 16:58:26 UTC |
| self.refresh_handler() | ||
|
|
||
| def refresh_handler(self): |
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.
what's the need for the refresh_handler to be a method rather than just inlined into __init__?
or what's the harm in just adding to JSONCodec on the import of this module?
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.
what's the need for the refresh_handler to be a method rather than just inlined into init?
In principle, the codecs in JSON_HANDLER can change. For example, an external package can register additional handlers. Since So we at least want the ability to refresh. In fact, I was thinking about whether the encoder/decoder properties maybe should call refresh_handler.
or what's the harm in just adding to
JSONCodecon the import of this module?
I'm not sure what you mean by "adding to JSONCodec." Could you clarify?
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.
Sorry I meant JSON_HANDLER. Other Codecs are defined and just appended onto the HANDLER as they are defined, so the HANDLER is "batteries included". Is there a reason this Codec can't have the same treatment?
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.
- We don't know the storage paths at import time, so this is literally impossible.
- We may have more than one set of
shared/permanent(especiallypermanent) defined in a given process. Imagine that I'm working with data from multiple campaigns. Imagine furthermore that I've use thekeep_sharedoption so that my shared stuff is also still accessible. This precludes any kind of serialization that relies on global state. This test is specifically to prevent us from making this mistake.
Adds a user story test where a type supported by a custom JSON codec is added to the result dict, and that codec isn't registered as of serialization object creation.
This is a subset of the work in #234, split off to facilitate review. This PR will merge into #258 (making it easier to see the additions specific to the PR, but meaning that this should be merged before that)
This handles the serialization of
StagingPathobjects. (detailed description will be coming in a future edit)TODO:
StagingSerializationis created.