-
Notifications
You must be signed in to change notification settings - Fork 20
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
Improve test comments under @pytest.mark.parametrize
in test_diffraction_objects.py
#273
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #273 +/- ##
=======================================
Coverage 98.68% 98.68%
=======================================
Files 8 8
Lines 379 379
=======================================
Hits 374 374
Misses 5 5
|
@sbillinge ready for review. @yucongalicechen , the part that is remaining is the The existing comments like |
(Just added a quick fix) pls see my comments right above |
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.
please see my comment. I guess this may not be liked by one of our linters or something, but I think I prefer the more compact formatting. Not a huge deal though. Let's just decide which we like and adopt it in future. It is such a small point we don't need to redo things here. Let's dicsus it. But I will merge this.
(4 * np.pi, np.array([0.25, 0.5, 0.71]), np.array([1, 2, 3]), "q", "q", 0.1, [0]), | ||
(4 * np.pi, np.array([30, 60]), np.array([1, 2]), "tth", "tth", 63, [1]), | ||
# Test get_array_index() returns the expected index given xtype and value | ||
# C1: Target value is in the xarray and xtype is identical, expect exact index match |
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.
let's not modify this PR again, but I do wonder whether it would be more compact as
( # C1: Target value is in the xarray and xtype is identical, expect exact index match
{
"wavelength": 4 * np.pi,
"xarray": np.array([30.005, 60]),
"yarray": np.array([1, 2]),
"xtype": "tth",
},
{
"xtype": "tth",
"value": 30.005,
},
[0],
),
than the current
# C1: Target value is in the xarray and xtype is identical, expect exact index match
(
{
"wavelength": 4 * np.pi,
"xarray": np.array([30.005, 60]),
"yarray": np.array([1, 2]),
"xtype": "tth",
},
{
"xtype": "tth",
"value": 30.005,
},
[0],
),
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.
@sbillinge yeah I like that! The automatic indentation also provides a good visual hierarchy!
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.
Some battle-tested practices being adopted:
cc' @yucongalicechen