-
Notifications
You must be signed in to change notification settings - Fork 18
861 refactor standard profiles should subclass profile 1 #872
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 1 #872
Conversation
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 734-refactor-steel-profiles #872 +/- ##
===============================================================
Coverage ? 100.00%
===============================================================
Files ? 404
Lines ? 12485
Branches ? 0
===============================================================
Hits ? 12485
Misses ? 0
Partials ? 0 β 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 to use a ProfileEnum base class that allows enum members to behave as Profile instances. The refactoring simplifies the architecture by making enum members actual Profile instances rather than requiring conversion methods, and enables the use of dataclass operations like transform on enum members.
Key changes:
- Introduces a new ProfileEnum base class that handles enum member creation and method overriding
- Refactors CHS standard profiles to directly instantiate CHSProfile objects as enum values
- Replaces
from_standard_profileclass method with instance methodwith_corrosionon CHSProfile - Updates Profile base class to use dataclass
replacefunction for simpler transformations
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| blueprints/structural_sections/steel/standard_profiles/profile_enum.py | New base class implementing enum mechanics for Profile subclasses with custom __new__, __init__, and transform methods |
| blueprints/structural_sections/steel/standard_profiles/chs.py | Refactored to define CHS enum members as direct CHSProfile instances, eliminating boilerplate initialization code |
| blueprints/structural_sections/steel/profile_definitions/chs_profile.py | Changed from class method from_standard_profile to instance method with_corrosion, renamed field from name to profile_name with property accessor |
| blueprints/structural_sections/_profile.py | Simplified transform method using dataclass replace function instead of manual dictionary manipulation |
| tests/structural_sections/steel/standard_profiles/test_chs.py | Updated tests to work with new enum-as-instance paradigm, changed method calls from as_cross_section to direct attribute access |
| tests/structural_sections/steel/profile_definitions/test_chs_profile.py | Updated tests to use with_corrosion instance method instead of from_standard_profile class method, added new test cases |
| tests/structural_sections/steel/profile_definitions/conftest.py | Simplified fixture to return CHS enum member directly instead of converting to CHSProfile |
Comments suppressed due to low confidence (1)
blueprints/structural_sections/steel/profile_definitions/chs_profile.py:83
- The documentation for the
corrosion_insideparameter is inconsistent with the implementation. The docstring states "Corrosion thickness to be added to the inner diameter" but the code actually subtracts it from the wall thickness. The parameter represents corrosion on the inside surface that reduces wall thickness, not something that increases the inner diameter. The documentation should say "Corrosion thickness on the inside surface to be subtracted from the wall thickness [mm] (default: 0)."
corrosion_inside : MM, optional
Corrosion thickness to be added to the inner diameter [mm] (default: 0).
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def transform(self, horizontal_offset: MM = 0, vertical_offset: MM = 0, rotation: DEG = 0) -> Self: | ||
| """Specialized transform method for ProfileEnum. It returns a new profile with the applied transformations. | ||
| Note | ||
| ---- | ||
| Calling the base class transform method directly would lead to errors, since the base class expects a pure dataclass instance. | ||
| If we called `super().transform(...)`, it would pass the Enum instance to the base class method, which is not compatible. | ||
| This method uses the underlying Profile instance's transform method on the enum member's value. | ||
| Enum member's value is a pure Profile instance, so we can call its transform method directly. | ||
| Parameters | ||
| ---------- | ||
| horizontal_offset : MM | ||
| Horizontal offset to apply [mm]. Positive values move the centroid to the right. | ||
| vertical_offset : MM | ||
| Vertical offset to apply [mm]. Positive values move the centroid upwards. | ||
| rotation : DEG | ||
| Rotation to apply [degrees]. Positive values rotate counter-clockwise around the centroid. | ||
| Returns | ||
| ------- | ||
| Self | ||
| New profile with the applied transformations. | ||
| """ | ||
| return self.value.transform(horizontal_offset, vertical_offset, rotation) |
Copilot
AI
Dec 14, 2025
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.
The return type annotation for the transform method is incorrect. The method returns self.value.transform(...), which returns a Profile instance (e.g., CHSProfile), not a ProfileEnum instance. The return type should be the underlying Profile type, not Self (which refers to the ProfileEnum subclass). This could cause type checking issues and mislead users about what type is returned. Consider changing the return type to match the actual Profile subclass type or document this behavior clearly.
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.
I like the clever implementation,.Only concern I have (after thinking more about it) is that it feels like we are misusing an Enum for something that it was not invented for --> and aren't we adding a lot of complexity?
Let's discuss it so we provide a clear solution for the end user.
KR
| from blueprints.type_alias import DEG, MM | ||
|
|
||
|
|
||
| class ProfileEnum(Enum): |
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.
Although your implementation is very clever: I'm a bit hessitant if an enum is the right choice now. --> Aren't we misusing the Enum class for something that it isn't designed for?
Originally the enum was a parameter container. But now we are moving to a classmethod without attributes.
The only reason now why we would like to use an enum is that the signature would be :
IPE.IPE300 # on the specific library class
instead of
IProfile.IPE300() #with brackets at the profile that it creates
For me feels that now we are adding a complex layer. Doesn't it make sense to have jus one class for ALL Iprofiles and also add some class methods there? Maybe we can even think of a clever getattr for a class attribute that acts like a classmethod if it exists in a dict of attributes, something like:
class Profile:
@classmethod
def from_library(cls, name: str) -> "Profile":
...
@classmethod
def __getattr__(cls, name: str) -> "Profile":
if name in PROFILE_DB:
return cls.from_library(name)
raise AttributeError(name)downside of that would be we lose intellisense (but we can think of solutions for that, like automated stubs or something else)
The benefit would be: we only have one class to maintain for an profile type, and especially for an enduser is it a lot clearer to have just one interface. I think it is more explicit.
I'm sorry if this comment turns out too disruptive, but I rather would do this well at once (or actually twice) than having to refactor again.
Description
Hi there,
β The bulk of this PR comes from a boilerplate kind of change. This is actually a small PR! π
So, my work in the previous PR resulted in some problems when I wanted to fully adopt the idea.
The problems:
transformmethod and possibly any future method that needs to create a copy of a Profile instance. The reason is that enum members are Singletons created at class definition. My custom enum was also not a real dataclass (it had lots of enum attributes), so thetransformmethod could not extract the required fields.I had to go down the
Enumrabbithole to figure out how to solve these problems. Fortunately, there is a clean solution:This PR solves the problems by:
\_\_new\_\_and\_\_init\_\_so that these do not need to be implemented in each subclass.transformmethod which passes a pure dataclass tosuper().transform().\_\_init\_\_.pyfile because that one calls all the enum classes and creates all enum member that we have ever defined!)β¨ In this PR, I have fully applied the new idea to
CHSstandard profile as my testcase!Fixes #861