-
Notifications
You must be signed in to change notification settings - Fork 24
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
Fix gc port type #378
Fix gc port type #378
Conversation
joamatab
commented
Apr 30, 2024
- update port_type
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 @joamatab - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -237,7 +237,7 @@ def gc_te1310() -> gf.Component: | |||
name = prefix_te1310 | |||
c.add_port( | |||
name=name, | |||
port_type=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.
suggestion (code_refinement): Consistency in port_type naming
Changing the port_type from a dynamic name to a static string 'vertical_te' improves consistency across component definitions. Ensure that this change aligns with the intended use cases and does not affect functionality where dynamic naming was required.
port_type=name, | |
port_type="vertical_te", |
@@ -816,6 +815,7 @@ | |||
|
|||
if __name__ == "__main__": | |||
c = straight_heater_metal() | |||
c.pprint_ports() |
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.
issue (code_refinement): Duplicate method call to pprint_ports()
The method 'pprint_ports()' is called twice consecutively. If this is not intentional, consider removing one of the calls to avoid redundancy.
@@ -824,7 +824,7 @@ | |||
# c = ring_double(length_y=10) | |||
# c = ring_with_crossing() | |||
# c = mmi1x2() | |||
# c = add_fiber_array(mzi) | |||
c = add_fiber_array(straight_heater_metal) |
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): Test case needed for add_fiber_array
with straight_heater_metal
.
The change in the argument passed to add_fiber_array
from mzi
to straight_heater_metal
could affect the function's behavior. Please add a test to verify that add_fiber_array
functions correctly with the new argument.
c = add_fiber_array(straight_heater_metal) | |
import pytest | |
def test_add_fiber_array_with_straight_heater_metal(): | |
component = straight_heater_metal() | |
result = add_fiber_array(component) | |
assert isinstance(result, Component), "Result should be an instance of Component" | |
assert result.name == "fiber_array", "Component name should be 'fiber_array'" |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #378 +/- ##
==========================================
- Coverage 69.58% 69.31% -0.28%
==========================================
Files 19 19
Lines 753 756 +3
==========================================
Hits 524 524
- Misses 229 232 +3 ☔ View full report in Codecov by Sentry. |