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 Mosviz Zoom Bug Typo #1992

Merged
merged 5 commits into from
Feb 7, 2023
Merged

Fix Mosviz Zoom Bug Typo #1992

merged 5 commits into from
Feb 7, 2023

Conversation

duytnguyendtn
Copy link
Collaborator

@duytnguyendtn duytnguyendtn commented Feb 7, 2023

Description

Oops...

Pretty alarming we didn't have a test for this. I added a quick and dirty one just to get this in ASAP.

Change log entry

  • Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
    list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
    should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • Do the proposed changes follow the STScI Style Guides?
  • Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • Did the CI pass? If not, are the failures related?
  • Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@duytnguyendtn duytnguyendtn added 🔥 Critical bug Something isn't working labels Feb 7, 2023
@duytnguyendtn duytnguyendtn added this to the 3.2.2 milestone Feb 7, 2023
Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

😬

jdaviz/configs/mosviz/tests/test_parsers.py Outdated Show resolved Hide resolved
@pllim pllim added the 💤 backport-v3.2.x on-merge: backport to v3.2.x label Feb 7, 2023
Co-authored-by: P. L. Lim <[email protected]>
jdaviz/configs/mosviz/tests/test_parsers.py Outdated Show resolved Hide resolved
jdaviz/configs/mosviz/tests/test_parsers.py Outdated Show resolved Hide resolved
duytnguyendtn and others added 2 commits February 7, 2023 17:19
@codecov
Copy link

codecov bot commented Feb 7, 2023

Codecov Report

Base: 91.96% // Head: 91.97% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (10fbec3) compared to base (3fc9a92).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1992   +/-   ##
=======================================
  Coverage   91.96%   91.97%           
=======================================
  Files         140      140           
  Lines       15315    15322    +7     
=======================================
+ Hits        14085    14092    +7     
  Misses       1230     1230           
Impacted Files Coverage Δ
jdaviz/configs/mosviz/helper.py 87.47% <100.00%> (ø)
jdaviz/configs/mosviz/tests/test_parsers.py 99.13% <100.00%> (+0.05%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pllim pllim merged commit 0793f81 into spacetelescope:main Feb 7, 2023
@lumberbot-app

This comment was marked as resolved.

@pllim
Copy link
Contributor

pllim commented Feb 7, 2023

I don't think this needs backporting at all. I don't see this error in 3.2.x code. Maybe we don't need a new release for Webbminar after all?

ra = table_data["R.A."][msg.selected_index]
dec = table_data["Dec."][msg.selected_index]

@pllim pllim modified the milestones: 3.2.2, 3.3 Feb 7, 2023
@pllim pllim removed Still Needs Manual Backport 💤 backport-v3.2.x on-merge: backport to v3.2.x labels Feb 7, 2023
@pllim
Copy link
Contributor

pllim commented Feb 7, 2023

Ah, this bug was introduced in #1958 that was never backported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🔥 Critical mosviz
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants