Skip to content
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 issues flagged by SonarQube #8391

Merged

Conversation

zacharyburnett
Copy link
Collaborator

@zacharyburnett zacharyburnett commented Mar 27, 2024

closes #8390

This PR addresses issues flagged by SonarQube (https://plsonarqube.stsci.edu/project/issues?resolved=false&inNewCodePeriod=true&types=BUG&id=jwst&open=AY54FDbH6KYUncGRZ448)

Checklist for maintainers

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • Make sure the JIRA ticket is resolved properly

@zacharyburnett
Copy link
Collaborator Author

SonarScan also found some issues here: #8390

@zacharyburnett zacharyburnett marked this pull request as ready for review March 27, 2024 14:48
@zacharyburnett zacharyburnett requested a review from a team as a code owner March 27, 2024 14:48
@zacharyburnett zacharyburnett self-assigned this Mar 27, 2024
@zacharyburnett
Copy link
Collaborator Author

regtests at 90% with no failures

@braingram
Copy link
Collaborator

braingram commented Mar 27, 2024

Thanks for opening the PR. As all the changes are in the new niriss AMI code and haven't so far triggered errors (which they should given the types of errors) I expect they are uncovered. Indeed I see no call to fit_image. EDIT fit_image is called. I missed

nrm.fit_image(self.ctrd,

@rcooper295 is NrmModel.fit_image ever used without providing modelin?

def fit_image(

If not I'd say we remove the method block of code.

Also the regtests will need to be run against CRDS-TEST to test the AMI code until ops is updated. It looks like the current run is using ops so the AMI tests are expected to fail.

Copy link
Collaborator

@braingram braingram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran the AMI regtests and unit tests with this branch locally (with a cache of CRDS-TEST files). The ami regtests pass with these changes.

Perhaps a follow-up issue should be opened to track the wider code cleanup with input from @rcooper295?

@zacharyburnett zacharyburnett merged commit bffe339 into spacetelescope:master Mar 27, 2024
24 checks passed
@zacharyburnett zacharyburnett deleted the fix/sonarqube_flags branch March 27, 2024 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sonarqube scan findings
3 participants