Fix test_with_attrfile bug and re-enable test #194
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
e10harvey
commented
Dec 12, 2024
| @@ -62,6 +62,33 @@ def parsers(self): | |||
| def _register_parser_class(cls, parser_class: type[aap.AbstractAttributeParser]): | |||
| _registered_parser_classes.add(parser_class) | |||
|
|
|||
| def get_subclass_parser_classes(self, parser_class: type[aap.AbstractAttributeParser]): | |||
There was a problem hiding this comment.
This method is not used nor required for our test suite to pass, remove it or add a unit test for it?
braden6521
previously approved these changes
Dec 13, 2024
There was a problem hiding this comment.
Looks good @e10harvey. I'm not too familiar with AttributeManager.py, so I'm not really sure of the implications of removing get_subclass_parser_classes. But I'm a fan of removing things that aren't used/tested; so that's my vote.
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.
Purpose
Fixes #3
Summary of changes
Broke subclass parser search in get_parser out into separate function that returns a list of all subclass parsers found
Implementation notes
An unordered set the type of the global variable
_registered_parser_classes. What causes the bug is the unordered nature of processing this set. Each implementation of the abstractAttributeParserregisters itself when opencsp fires up. WhenSpotAnalysisAttributeParseris encoutered first duringAttributeManager/get_parser, the unordered structure of_registered_parser_classescan result in the fall-through path to search and returning a subclass parser. This in-turn results in some members of the parser,date_collectedin this instance, not getting initialized. Removing the fall back path of searching for subclass parsers inget_parserfixes this spurious failure.Submission checklist
develop, notmainopencsp/test/test_DocStringsExist.pyare verified to include this change or have been updated accordinglydoc/are verified to include this change or have been updated accordinglyAdditional information
It's not clear to me what the original intended functionality of
get_parserwas. Reviewers should check the doc strings and any potential breakage via removal of subclass parser searching.