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
Data objects #12
Data objects #12
Changes from all commits
6b47076
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.
@RonnyPfannschmidt - okay, try this now. I think formalising the
Proxy
object gives a really nice clean outcome, but interested to hear what you think. If you want, I think you should be fine to stick the proxy object on the config root too :-)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.
from my pov this is basically a drunk identity copied contextvar,
hence the desire to access the config root and path of the dynamic lookup
the tricky point about the client landing in data however is a valid concern, however i would currently consider it less of a problem than having the loader proxy object be integrated differently
how about the concept of a
Sidechannel
valueit would be a
Contant Data Value
that one could update to a object for within a Config, but uppon copying a config, it would"loose" the Value
then it would no longer be a contextvar, but rather a symblic reference one could map to other objects
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.
"from my pov this is basically a drunk identity copied contextvar," comes across quite badly, I'm trying to help here. I don't need this functionality myself, and I'm struggling to understand the resistance to the suggestions I'm making.
I haven't used
ContextVar
so maybe this is a good opportunity for me to learn.I can't get my head around your
Con(s?)tant Data Value
idea, but it seems to be related to your fixation with storingvault_loader
on theConfig
object. That's not something I really like, but I'd like to understand why it's important to you so that we can find something that keeps both of us happy :-)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 would be fine with not storing the vault loader on the config,
a key consideration for me is how to make it available to the loaded value (which ideally doesn't need a extra magical object
having a proxy object thats tricky to manage and weaseling it into a dynamic loader is something i would like to avoid
im under the impression we have a very different understanding of how to pass the context in (my preference would be to just pass the root config object)
your preference seems to be to pass in something like a context variable, however the proposed variant looks to me like it will carry context over into new copies of the configuration (which seems to be a bug NO)
the idea of the "standin values" would be to have a way to have a sidechannel for the configruation
so that if one had a config like
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.
@RonnyPfannschmidt - I hope it's not intentional, but your tone is still coming across quite negatively... "magical", "weaseling" and "bug NO" are not things that make this fun for me to work on.
I just read up on
ContextVar
, and while the API may appear similar, for me, theProxy
here is more like a TwistedDeferred
. It's a proxy for an object because we don't have that object when we need to pass around a reference to it. They only need to be referenced in the config parsing code, so not sure how that equates to being tricky to manage. The dynamic loader needs a reference to the vault client before the vault client can be instanted, so having a proxy reference to it which case be filled in later felt quite natural to me.Unfortunately, the notion of when to carry over a Proxy into a new config is a tricky one. I'm interested in where that would cause a problem?
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.
Most likely the same way as in symbol, which is just a name for proxy
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.
get_context
in your example appears to only be needed if you require path traversal, which I think we both agreed was a bad thing? (The path can be out of sync with where the node is in the config).I'm trying to figure out a way the
Proxy
can have a value per config without needing a direct reference to the config, since I'm still not sure that can be plumbed through: configurator has always tried to not get involved with the underlying config data (although I think #9 may have to change that...), so.data
never has access to a node, it's always theConfigNode
that refers down to the.data
, and the nodes are basically ephemeral.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.
it cannot be done with
get
-get_context
is intended to pass the config trough to each constructed node, so it can be used in the lazy resolvingThere 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, specifically, cannot be done with
get
?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.
Obtaining the config object that was in use for the lookup