Remove the target keyword from rdm_open
#235
Merged
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
WilliamJamieson
commented
Jul 7, 2023
| of the model. In this case the constructor should just directly return | ||
| the model. | ||
| """ | ||
| if init.__class__.__name__ == cls.__name__: |
There was a problem hiding this comment.
Bad things happen (infinite recursion) if one tries to isinstance checks here.
WilliamJamieson
force-pushed
the
feature/pass_like_to_datamodel_constructor
branch
from
b722011 to
402c887
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #235 +/- ##
==========================================
+ Coverage 94.47% 95.00% +0.53%
==========================================
Files 23 23
Lines 1628 1621 -7
==========================================
+ Hits 1538 1540 +2
+ Misses 90 81 -9
☔ View full report in Codecov by Sentry. |
WilliamJamieson
force-pushed
the
feature/pass_like_to_datamodel_constructor
branch
from
402c887 to
23d5233
Compare
WilliamJamieson
force-pushed
the
feature/pass_like_to_datamodel_constructor
branch
from
23d5233 to
100e32e
Compare
Issue spacetelescope#51 and PR spacetelescope#52 indicate that we should be able to pass a model instance to a matching model's constructor, and then return exactly the same instance unmodified.
WilliamJamieson
force-pushed
the
feature/pass_like_to_datamodel_constructor
branch
from
100e32e to
e0bcccd
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 PR addresses issue #51 and the partial solution to it in PR #52. The solution in #52 was to add the
targetkeyword tordm_open, however it did not fix the actual issue from #51. Then in PR #59, the tests for the solution in #52 were completely removed, and after that point thetargetargument completely ceased being used inroman_datamodelsandromancal.The issue at hand in #51 is that if we pass a data model instance to the datamodel constructor (
<data_model_class>(*args, **kwargs)), then we don't want it to raise an error. The discussion and solution in #52 additionally indicates the we should pass the exact object out with no alterations rather than constructing a new object.This pull request adds a test reproducing the error described in #51 together with the additional constraints from #52. It then solves the issue via injecting a new
__new__method for theDataModelobjects which detects and then handles this case special case. It then reverts the partial solution from #52.Checklist
CHANGES.rstunder the corresponding subsection