-
Notifications
You must be signed in to change notification settings - Fork 20
PART 2 - 957 feature request production method in steel cross section #959
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
Conversation
|
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
📝 WalkthroughWalkthroughA new public field Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
hi @SZeltaat, id like to add production_method as an attribute to the steel cross section, so that i can diff between option a) and d) for shear area
Could you give it a check? thanks! |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 840-feature-request-add-first-steel-check-to-blueprints #959 +/- ##
=========================================================================================
Coverage 100.00% 100.00%
=========================================================================================
Files 422 422
Lines 13145 13189 +44
=========================================================================================
+ Hits 13145 13189 +44 ☔ 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 adds support for specifying the fabrication method (rolled or welded) for steel cross-sections, addressing feature request #957. The implementation adds a new optional parameter with a default value of "rolled" to maintain backward compatibility.
Changes:
- Added a
fabrication_methodattribute to theSteelCrossSectionclass with typeLiteral["rolled", "welded"]and default value "rolled" - Added test coverage to verify the fabrication method functionality
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| blueprints/structural_sections/steel/steel_cross_section.py | Added fabrication_method attribute with type hint and default value |
| tests/structural_sections/steel/test_steel_cross_section.py | Added test case to verify default fabrication method is "rolled" |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Hi Gerjan,
I think this is the right place for this attribute, but we should be more carefull with the default. Please see my 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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
I think this can be implemented in a much better way. Now, we look for profile's name in the whole database. We can narrow it down more effectively.
In any case, we should have good test cases to test this functionality.
- For hot rolled
- For cold-formed
- For edge cases where name is not found, etc... (so every if-condition)
- For tricky situations like RHS and RHSCF
If all test cases pass, we can always change the implementation later.
My suggestion for the implementation:
First extract only the first alphabetic part of the name.
Find the corresponding profile class by matching against the cls.name
Then extract the profile name (like you already do, alternatively use the pattern alphabetic, followed by multiple numerics optionally seperated by "x")
Then if any(profile_name == standard_profile.name for standard_profile in cls._database.values()), assign hot-rolled / cold-formed based on cls.name.
…itional test fixtures
…nd corresponding test
|
For hot rolled -> done First extract only the first alphabetic part of the name. |
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
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ction' of https://github.com/Blueprints-org/blueprints into 957-feature-request-production_method-in-steel-cross-section
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
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… fabrication method and profile extraction
…' into 957-feature-request-production_method-in-steel-cross-section
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
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
LGTM!
5f48d14
into
840-feature-request-add-first-steel-check-to-blueprints

Description
feature request production method in steel cross section
Fixes #957
Type of change
Checklist:
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.