tickets/DM-33977 Fixes for ConfigurableActionStructField #655
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TallJimbo
approved these changes
Mar 24, 2022
| @@ -237,6 +247,21 @@ def __set__(self, instance: Config, | |||
| if value is None or value == self.default: | |||
| value = self.StructClass(instance, self, value, at=at, label=label) | |||
| else: | |||
| # An actual value is being assgigned check for what it is | |||
There was a problem hiding this comment.
Suggested change
| # An actual value is being assgigned check for what it is | |
| # An actual value is being assigned, check for what it is |
| elif isinstance(value, (SimpleNamespace, Struct)): | ||
| # If this is a a python analogous container, we need to make | ||
| # a ConfigurableActionStruct initialized with this data | ||
| value = self.StructClass(instance, self, vars(value), at=at, label=label) |
There was a problem hiding this comment.
Are you sure that the call to vars doesn't return any "special" attributes for either SimpleNamespace or Struct? Seems like something worth adding a test for.
There was a problem hiding this comment.
I verified it though with a few tests in python manually, but I will add a unit test to guard against behavior changes (especially in Struct)
natelust
force-pushed
the
tickets/DM-33977
branch
2 times, most recently
from
70dc373
to
af701d9
Compare
The ConfigurableActionStructField needs to be able to take appropriate assignements and either make a copy, or default construct an instance of ConfigurableActionStruct, otherwise the reference to the field object is lost.
If one adds attributes through a configurable action struct through the update interface it was possible to use a string that was not a valid python variable name, only to fail later when it was read or loaded through a config file. This validates that the strings are appropriate identifiers.
Add unit tests to test the behaviors when assigning to a ConfigurableActionStructField.
natelust
force-pushed
the
tickets/DM-33977
branch
from
a7dbb50
to
a98ff70
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This ticket fixes some assignment logic in ConfigurableActionStructField wherein a local copy of a ConfigurableActionStruct was not being made, leaving a dangling reference to a config object that goes out of scope.
This ticket also makes a change to stay in line with other pex_config fields wherein it takes other python classes that duck like a ConfigurableActionStructField as an argument in assigment.
Finally this ticket adds a verification that a string used in the dict update interface is a valid python identifier.