-
Notifications
You must be signed in to change notification settings - Fork 167
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
remove unused code causing sonar scan violation #8471
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8471 +/- ##
=======================================
Coverage 57.93% 57.93%
=======================================
Files 387 387
Lines 38839 38852 +13
=======================================
+ Hits 22502 22510 +8
- Misses 16337 16342 +5 ☔ View full report in Codecov by Sentry. |
Thanks @braingram! A few questions:
Considering there are only ~7 failing instances in the entire jwst repo, it doesn't seem like turning the rule on would affect people's coding workflow much; that is, while there might be known issues, they don't seem very common. If B018 does indeed catch the same problems as the sonar scan, and if there are no instances where fixing the code is onerous (and therefore would annoy us when writing new, similar code) then I'm all for turning it on and fixing these. |
Thanks for giving this a look! B018 does catch the wfss_contam line flagged by sonar scan. I'm new to sonar qube and looking at it's output so I'm not certain there are other places where it's flagging lines for S905 errors. I think it also has restrictions on where it can run (due to licensing) but it's part of the "up and coming" github actions regression test system. One example of a line flagged by B018 is here: Lines 235 to 241 in 241c5c0
The attribute in the try block violates B018. It's likely this could be changes to hasattr but I'd have to look at the details of the model implementation to know if that is equivalent. Similarly, assign_wcs has the following:Lines 106 to 109 in 9e857a5
Note that it's looking for a NotImplementedError so I don't expect a hasattr works here.
As sonar qube seems to be a linter and jwst already uses ruff I was hoping the rules would be similar enough that we could catch errors with either tool. If possible, I think disabling the S905 checking might be an easier solution for the jwst developers. I'd be happy to discuss my opinions on ruff if more rules are being considered for jwst. As a point of reference astropy has seen some feedback criticizing it's rather extensive adoption of ruff rules: |
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.
The singular code change looksgood to me. I guess @hbushouse should chime in about the ruff check, but I agree with your logic
It looks like sonar scan is now flagging many more lines which this PR does not address: #8493 I'm going to close this as it's only a partial fix. @zacharyburnett let me know if this interferes with any of the regtest testing you were doing with this PR. |
Sonar scan (required for the github actions regression tests) is failing due to a S905 violation (I didn't find any corresponding ruff rule but maybe someone else can?):
https://plsonarqube.stsci.edu/project/issues?resolved=false&inNewCodePeriod=true&types=BUG&branch=main&id=jwst&open=AY9VhfDklU1kwdd-cobK
This PR removes the line of code that has no side effects.
B018 looks similar but also fails the following:
Some of these do look "useless" but some are not. I'd rather not open the flood gates of tuning ruff rules but would also like to find a solution that allows catching these sonar scan issues before they're merged. If it seems worth adding B018 I am happy to fix the above failures (or add noqas).
Checklist for maintainers
CHANGES.rst
within the relevant release sectionHow to run regression tests on a PR