-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add aluminum sections #12
Conversation
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.
Hey @youandvern - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 4 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
def get_aluminum_angle(section_size: str) -> AluminumAngle: | ||
""" | ||
Fetches the properties of a specified Aluminum Angle section from the sections database and returns a | ||
:class:`efficalc.sections.AluminumAngle` instance populated with these properties. | ||
|
||
:param section_size: The designation of the aluminum section size as the Size property defined in the database. | ||
:type section_size: str | ||
:return: An AluminumAngle populated with the properties of the specified section size. | ||
:rtype: `efficalc.sections.AluminumAngle` | ||
:raises ValueError: If the specified section size cannot be found in the sections database. | ||
""" | ||
|
||
row = _fetch_aluminum_section_row(ALUM_ANGLE_TABLE, section_size) | ||
|
||
if row: | ||
return AluminumAngle(**row) | ||
else: | ||
raise ValueError( | ||
f"The aluminum angle section size named {section_size} could not be found." |
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.
suggestion (code_refinement): Consider consolidating aluminum section retrieval functions.
The functions for retrieving aluminum section properties (angle, channel, circular, rectangular, wide flange) are very similar. Consider refactoring to reduce code duplication, perhaps by using a more generic function that takes the section type as an argument.
@@ -100,3 +105,76 @@ def test_wide_flange_lookup_size_not_found(): | |||
with pytest.raises(ValueError) as e_info: | |||
get_aisc_wide_flange("NOT_FOUND") | |||
assert "section size named NOT_FOUND could not be found" in str(e_info.value) | |||
|
|||
|
|||
def test_aluminum_angle_lookup(): |
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.
suggestion (testing): Consider adding negative test cases for aluminum sections.
It's great to see positive test cases for aluminum sections. However, adding negative test cases where invalid inputs are provided could enhance the robustness of the tests. This could include scenarios where the section size does not exist in the database.
def test_aluminum_angle_lookup(): | |
def test_aluminum_angle_lookup_invalid_format(): | |
with pytest.raises(ValueError) as e_info: | |
get_aluminum_angle("INVALID_FORMAT") | |
assert "Invalid format for aluminum angle section" in str(e_info.value) | |
def test_aluminum_channel_lookup_invalid_dimensions(): | |
with pytest.raises(ValueError) as e_info: | |
get_aluminum_channel("C 0 X 0") | |
assert "Invalid dimensions for aluminum channel section" in str(e_info.value) |
@@ -100,3 +105,76 @@ | |||
with pytest.raises(ValueError) as e_info: | |||
get_aisc_wide_flange("NOT_FOUND") | |||
assert "section size named NOT_FOUND could not be found" in str(e_info.value) | |||
|
|||
|
|||
def test_aluminum_angle_lookup(): |
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.
suggestion (testing): Consider parameterizing tests for aluminum sections.
Given the similarity across tests for different aluminum section types, consider using pytest's parameterize feature. This could reduce code duplication and make it easier to extend tests in the future.
@@ -100,3 +105,76 @@ | |||
with pytest.raises(ValueError) as e_info: | |||
get_aisc_wide_flange("NOT_FOUND") | |||
assert "section size named NOT_FOUND could not be found" in str(e_info.value) | |||
|
|||
|
|||
def test_aluminum_angle_lookup(): |
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.
suggestion (testing): Missing test cases for edge conditions.
The current tests cover basic functionality but seem to miss edge conditions, such as extremely large or small section sizes. Adding these could ensure the system behaves correctly under all circumstances.
def test_aluminum_angle_lookup(): | |
def test_aluminum_angle_lookup_extreme_sizes(): | |
large_section = get_aluminum_angle("L 12 x 12 x 1") | |
assert large_section.d == 12 | |
assert large_section.b == 12 | |
assert large_section.t == 1 | |
small_section = get_aluminum_angle("L 0.5 x 0.5 x 0.125") | |
assert small_section.d == 0.5 | |
assert small_section.b == 0.5 | |
assert small_section.t == 0.125 |
@@ -100,3 +105,76 @@ | |||
with pytest.raises(ValueError) as e_info: | |||
get_aisc_wide_flange("NOT_FOUND") | |||
assert "section size named NOT_FOUND could not be found" in str(e_info.value) | |||
|
|||
|
|||
def test_aluminum_angle_lookup(): |
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.
suggestion (testing): Ensure consistency in test data.
The test data used across different test cases for aluminum sections should be consistent unless testing specific scenarios. This helps in maintaining clarity and reliability of the tests.
row = _fetch_aluminum_section_row(ALUM_CIRCULAR_TABLE, section_size) | ||
|
||
if row: |
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.
suggestion (code-quality): Use named expression to simplify assignment and conditional (use-named-expression
)
row = _fetch_aluminum_section_row(ALUM_CIRCULAR_TABLE, section_size) | |
if row: | |
if row := _fetch_aluminum_section_row(ALUM_CIRCULAR_TABLE, section_size): |
row = _fetch_aluminum_section_row(ALUM_RECTANGULAR_TABLE, section_size) | ||
|
||
if row: |
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.
suggestion (code-quality): Use named expression to simplify assignment and conditional (use-named-expression
)
row = _fetch_aluminum_section_row(ALUM_RECTANGULAR_TABLE, section_size) | |
if row: | |
if row := _fetch_aluminum_section_row( | |
ALUM_RECTANGULAR_TABLE, section_size | |
): |
row = _fetch_aluminum_section_row(ALUM_WIDE_FLANGE_TABLE, section_size) | ||
|
||
if row: |
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.
suggestion (code-quality): Use named expression to simplify assignment and conditional (use-named-expression
)
row = _fetch_aluminum_section_row(ALUM_WIDE_FLANGE_TABLE, section_size) | |
if row: | |
if row := _fetch_aluminum_section_row( | |
ALUM_WIDE_FLANGE_TABLE, section_size | |
): |
contents = "" | ||
contents += "(" |
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.
suggestion (code-quality): Replace assignment and augmented assignment with single assignment (merge-assign-and-aug-assign
)
contents = "" | |
contents += "(" | |
contents = "" + "(" |
contents = "" | ||
contents += "(" |
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.
suggestion (code-quality): Replace assignment and augmented assignment with single assignment (merge-assign-and-aug-assign
)
contents = "" | |
contents += "(" | |
contents = "" + "(" |
No description provided.