Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

added missing import statement for logging to enable error messages i… #2710

Conversation

caitwolf
Copy link
Contributor

…n a pop up window when slit width is set larger than slit length

Description

This PR returns the missing import statement for logging in the smearing widget.

Fixes #2708

How Has This Been Tested?

Simulated data using the sphere model in the GUI with slit smearing at the following three conditions:

  1. length=0.1, width=0
  2. length=0.1, width=0.1
  3. length=0, width=0.1
    Conditions 2 and 3 should result in an error message being presented in a pop-up window that explains the proper usage of the length and width terms for slit smearing. "CRITICAL" messages should also appear in the log explorer. Condition 1 should run normally and simulate the slit-smeared data.

Review Checklist (please remove items if they don't apply):

  • Code has been reviewed
  • Functionality has been tested
  • Windows installer (GH artifact) has been tested (installed and worked)
  • MacOSX installer (GH artifact) has been tested (installed and worked)
  • User documentation is available and complete (if required)
  • Developers documentation is available and complete (if required)
  • The introduced changes comply with SasView license (BSD 3-Clause)

…n a pop up window when slit width is set larger than slit length
@caitwolf caitwolf linked an issue Jan 16, 2024 that may be closed by this pull request
@wpotrzebowski
Copy link
Contributor

This PR is currently pointing towards main but do we also want to have this fix in release_6.0.0?

Copy link
Contributor

@lucas-wilkins lucas-wilkins left a comment

Choose a reason for hiding this comment

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

LGTM

@butlerpd
Copy link
Member

Agreed this should point to 6.0.0.

On another note - should we keep this open to use in the training on reviewing PR?

@lucas-wilkins lucas-wilkins merged commit 4f23e38 into main Jan 17, 2024
19 checks passed
@lucas-wilkins lucas-wilkins deleted the 2708-missing-import-statement-for-logging-in-smearingwidget branch January 17, 2024 05:13
@caitwolf caitwolf restored the 2708-missing-import-statement-for-logging-in-smearingwidget branch January 19, 2024 22:26
@caitwolf
Copy link
Contributor Author

Restoring this branch for now so we can include this in 6.0.0

@caitwolf
Copy link
Contributor Author

@butlerpd should we merge this change with the 6.0.0 release branch? This had gotten merged to main during contributor camp and I hadn't circled back to our above discussion to merge it in both

@caitwolf caitwolf deleted the 2708-missing-import-statement-for-logging-in-smearingwidget branch March 25, 2024 17:53
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing import statement for logging in SmearingWidget
4 participants