Move Platform class into preprocessor.py
#198
Merged
Conversation
This file contains hidden or 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
There was a circular dependency between platform.py and preprocessor.py: - Platform stores Macro and MacroFunction objects; and - MacroExpander requires a Platform. Breaking this dependency enables Platform arguments in preprocessor.py to be decorated with the correct type-hints. This code structure is also a better reflection of how Platform is used by the preprocessor. Signed-off-by: John Pennycook <[email protected]>
Pennycook
changed the title
Platform class into preprocessor.py
There was a problem hiding this comment.
Pull Request Overview
This PR resolves a circular import dependency between platform.py and preprocessor.py by moving the Platform class definition from platform.py to preprocessor.py, enabling proper type hints for Platform arguments in the preprocessor module.
- Move
Platformclass fromplatform.pytopreprocessor.py - Delete the now-empty
platform.pyfile - Update all import statements to use
preprocessor.Platforminstead ofplatform.Platform
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| codebasin/platform.py | File deleted entirely as Platform class moved to preprocessor.py |
| codebasin/preprocessor.py | Added Platform class definition and updated MacroExpander type hint |
| codebasin/finder.py | Updated imports to use Platform from preprocessor module |
| tests/operators/test_operators.py | Updated imports to use Platform from preprocessor module |
| tests/macro_expansion/test_macro_expansion.py | Updated imports to use Platform from preprocessor module |
Co-authored-by: Copilot <[email protected]> Signed-off-by: John Pennycook <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: John Pennycook <[email protected]>
Pennycook
force-pushed
the
refactor/platform-class
branch
from
3b713bd to
5091f4b
Compare
|
Accepting Copilot's changes didn't include the sign-off this time for some reason, so I've had to force-push. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
There was a circular dependency between platform.py and preprocessor.py:
PlatformstoresMacroandMacroFunctionobjects; andMacroExpanderrequires aPlatform.Breaking this dependency enables
Platformarguments in preprocessor.py to be decorated with the correct type-hints. This code structure is also a better reflection of howPlatformis used by the preprocessor.Related issues
Proposed changes
Platformdefinition from platform.py to preprocessor.pyplatform.Platformto usepreprocessor.Platforminstead.Reviewing the code, it is clear that a
Platformdoesn't really represent a "platform" in the sense we mean it elsewhere. It's really something like a "preprocessor state", and we construct a differentPlatformfor each file in a codebase. As a follow-up to this PR I intend to renamePlatformand tidy it up, but I wanted to make sure simply moving the class didn't break anything before going further.