- Notifications
You must be signed in to change notification settings - Fork 21
Refactor test_scale_to function - continue to establish good practices#255
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
bobleesj commented Dec 19, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
codecovbot commented Dec 19, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## main #255 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 8 8 Lines 380 378 -2 ========================================= - Hits 380 378 -2
|
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bobleesj commented Dec 19, 2024
@sbillinge ready for review - @yucongalicechen continuing to refactor test functions for future maintenance. |
sbillinge 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.
looks great....but please see inline.
src/diffpy/utils/transforms.py Outdated
| inf_output_wmsg = ( | ||
| "INFO: The largest output value in the array is infinite. This is allowed, but it will not be plotted." | ||
| ) | ||
| inf_output_wmsg = "The largest output value in the array is infinite. This is allowed, but it will not be plotted." |
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.
This is supposed to be an info not a warning. I think we want INFO and change wmsg to imsg in the name. Make sure it is a print and not a warning.warn in the code.
The group standard could be that a warning means that something is wrong, albeit not error generating.
Here it is "expected" that we will have infinities sometimes and we want to handle them if possible. So nothing is wrong but we want the user to alerted so they understand any magic.
Not sure if that all makes sense.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
tests/test_diffraction_objects.py Outdated
| }, | ||
| False, | ||
| ), | ||
| ( # One without wavelnegth, expect inequality |
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.
quick note to myself - fix typo
sbillinge commented Dec 19, 2024
@bobleesj LGTM. I am happy to merge whenever you are ready. Great improvement. Not sure if you did the high level scoping sttement. I have to board my flight though.... |
bobleesj commented Dec 19, 2024
@sbillinge I will take some time to internalize the comments and make new commits later today. Thanks a lot! Safe travels to Africa. |
bobleesj commented Dec 20, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@sbillinge Ready for review - the last commit is just quick fix from For higher-level ones, I created separate issues: 1) how to enforce BG members to define testing scope before writing one 2) grouping test cases 3) review warning/info/error runtime error messages |
8eaf5d7 into diffpy:mainUh oh!
There was an error while loading. Please reload this page.
bobleesj commented Dec 20, 2024
Noted the additional commits made with higher-level test comments. |
The goal of this PR is to demonstrate how to refactor a specific test function.
@pytest.mark.parametrizeto init objects