-
Notifications
You must be signed in to change notification settings - Fork 7
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
closes #430 #436
base: master
Are you sure you want to change the base?
closes #430 #436
Conversation
@@ -935,7 +935,7 @@ def __init__(self, property_owner, type_uri, reference_type_uri, | |||
super().__init__(property_owner, type_uri, lower_bound, upper_bound, | |||
validation_rules, initial_value) | |||
self.reference_type_uri = reference_type_uri | |||
if self._sbol_owner is not None: | |||
if self._sbol_owner is None: |
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.
If this adjusted branch is ever taken, it will break on line 940, because it will attempt to assign to None.properties
.
@tcmitchell : do you remember why we have this setting property_store to the empty list? It appears to be preventing initialization with init value from being done as expected in at least some cases.
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 do not.
I think _sbol_owner
is the pointer to the SBOL object that "owns" this object. It's possible this path was trying to mimic something in the C++ code.
I think if this change passes the new unit test, and this change does not fail any other unit tests, then it should be ok.
Hi @tcmitchell thanks for approving, but I think the if statement should be removed entirely. As @jakebeal if it would ever be true (may be impossible). Do you agree with this? |
Second attempt of #431, this one should pass tests after the fix from #435. closes #430