-
Notifications
You must be signed in to change notification settings - Fork 18
861 refactor standard profiles should subclass profile #879
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
861 refactor standard profiles should subclass profile #879
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 734-refactor-steel-profiles #879 +/- ##
=============================================================
Coverage 100.00% 100.00%
=============================================================
Files 410 410
Lines 12840 12840
=============================================================
Hits 12840 12840 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull request overview
This PR refactors the standard profiles implementation from an Enum-based approach to a metaclass-based approach to avoid eager initialization overhead. The CHS (Circular Hollow Section) profiles are now accessed through dynamic attribute lookup using StandardProfileMeta, returning CHSProfile instances on demand. Key changes include:
- Introduction of
StandardProfileMetametaclass for lazy profile instantiation - Refactoring of
CHSfrom Enum to a regular class with metaclass - Change of API:
profile.as_cross_section()replaced with direct profile access andprofile.with_corrosion() - Addition of
.pyistub file for type hints
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| blueprints/structural_sections/steel/standard_profiles/utils.py | Adds StandardProfileMeta metaclass and StandardProfileProtocol for dynamic profile attribute access |
| blueprints/structural_sections/steel/standard_profiles/chs.py | Refactors CHS from Enum to metaclass-based class with database dictionary |
| blueprints/structural_sections/steel/standard_profiles/chs.pyi | New stub file providing type hints for all CHS profile attributes |
| blueprints/structural_sections/steel/profile_definitions/chs_profile.py | Renames from_standard_profile to with_corrosion as an instance method |
| tests/structural_sections/steel/standard_profiles/test_chs.py | Simplifies tests but introduces tautological assertions and reduces coverage |
| tests/structural_sections/steel/profile_definitions/test_chs_profile.py | Updates tests to use new with_corrosion() API |
| tests/structural_sections/steel/profile_definitions/conftest.py | Simplifies fixture to directly return CHS profile instead of converting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
blueprints/structural_sections/steel/standard_profiles/utils.py
Outdated
Show resolved
Hide resolved
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Comment |
bro-wi
left a comment
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.
Great job! I think we are nearly there. Biggest consideration for me is maintaning two interfaces as now:
CHSProfile() and CHS.CHS21_3x2_3
compared to a single interface:
CHSProfile() and CHSProfile.CHS21_3x2_3
blueprints/structural_sections/steel/profile_definitions/chs_profile.py
Outdated
Show resolved
Hide resolved
SZeltaat
left a comment
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.
❤️
blueprints/structural_sections/steel/profile_definitions/chs_profile.py
Outdated
Show resolved
Hide resolved
…-profiles-should-subclass-profile
Description
Hi @egarciamendez. I have already discussed this with Wichard. The approach with Enums had a huge overhead that created all the standard profiles in our library, twice even if we only needed one standard profile. That is due to the nature of Enums. In addition, I was overriding the Enum Meta which is potentially a risk of unknown behaviour. I have tried 2 alternatives, 1: using descriptors (which had the same disadvantage of initiating all profiles eagerly on class definition) and 2: class getattr (this PR).
This approach has a slight disadvantage of creating the stubs, but I can accept that because:
This way we have all the advantages of user-friendly API and clean Pythonic code, without unnecessary overhead at runtime.
Could you please review this PR and let me know what you think?
If approved, I'll make the changes to all the other standard profiles.
Fixes #861
Type of change
Checklist: