Skip to content

TestMaskedQuantityInitialization: Fix initialization#15326

Merged
pllim merged 2 commits intoastropy:mainfrom
doronbehar:masked-test
Sep 18, 2023
Merged

TestMaskedQuantityInitialization: Fix initialization#15326
pllim merged 2 commits intoastropy:mainfrom
doronbehar:masked-test

Conversation

@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Sep 16, 2023

Description

Fixes #15316 .

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 16, 2023 •
edited by mhvk
Loading

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

github-actions[bot]
github-actions bot reviewed Sep 16, 2023
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to Astropy 👋 and congratulations on your first pull request! 🎉

A project member will respond to you as soon as possible; in the meantime, please have a look over the Checklist for Contributed Code and make sure you've addressed as many of the questions there as possible.

If you feel that this pull request has not been responded to in a timely manner, please send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail [email protected].

@doronbehar doronbehar mentioned this pull request Sep 16, 2023
Closed
@mhvk
Copy link
Contributor

mhvk commented Sep 16, 2023

Oops, so sorry, I missed this!! It is good except for a missing blank line after the class. I think that can be fixed auto-magically...

@mhvk
Copy link
Contributor

mhvk commented Sep 16, 2023

pre-commit.ci autofix

@mhvk mhvk added this to the v5.3.4 milestone Sep 16, 2023
mhvk
mhvk approved these changes Sep 16, 2023
View reviewed changes
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Ha, the auto-fix worked! So, approving and setting to merge when all tests pass! Thanks, @doronbehar!

@mhvk mhvk enabled auto-merge (squash) September 16, 2023 12:46
@mhvk mhvk mentioned this pull request Sep 16, 2023
Merged
@mhvk
Copy link
Contributor

mhvk commented Sep 16, 2023

Oops, unrelated RTD failure. I think this will need someone with override privilige -- @pllim??

@pllim
Copy link
Member

pllim commented Sep 16, 2023

I am a little confused why there are two different PRs now trying to address the same problem. I asked over at the original issue. Once I am not confused anymore, yes, I can help get them merged. 😸

@doronbehar
Copy link
Contributor Author

doronbehar commented Sep 16, 2023

@mhvk's PR includes my changes so we can merge this one, or perhaps better, merge @mhvk's PR which include my commit, pulled from my branch.

@pllim
Copy link
Member

pllim commented Sep 16, 2023

So I merge this first, and the mhvk has to rebase his other PR as a follow-up patch?

@doronbehar
Copy link
Contributor Author

doronbehar commented Sep 16, 2023

Yes that's also an option.

@pllim
Copy link
Member

pllim commented Sep 16, 2023

Thanks! I'll wait a bit and see if mhvk agrees. 😸

@mhvk
Copy link
Contributor

mhvk commented Sep 17, 2023

Indeed, my plan was to merge this (for which backporting is useful) and then rebase my follow-up. It just didn't work because of the RTD failure (which I cannot override). Now that that is fixed in main, I rebased and forced-push here, and set auto-merge, so there should be no more work here! Thanks, all!

@mhvk
Copy link
Contributor

mhvk commented Sep 17, 2023

Now held back by #15333... couldn't those failures at least have the decency to apear during the week???

@pllim pllim disabled auto-merge September 18, 2023 12:59
@pllim pllim merged commit b0065e8 into astropy:main Sep 18, 2023
@pllim
Copy link
Member

pllim commented Sep 18, 2023

I merged it. Please rebase the other PR. Thanks!

@meeseeksmachine meeseeksmachine mentioned this pull request Sep 18, 2023
Merged
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Sep 18, 2023
@doronbehar @meeseeksmachine
Backport PR astropy#15326: TestMaskedQuantityInitialization: Fix init…
d02a5ce 
…ialization
@mhvk
Copy link
Contributor

mhvk commented Sep 18, 2023

Thanks!

pllim added a commit that referenced this pull request Sep 18, 2023
@pllim
Merge pull request #15335 from meeseeksmachine/auto-backport-of-pr-15…
9124575 
…326-on-v5.3.x

Backport PR #15326 on branch v5.3.x (TestMaskedQuantityInitialization: Fix initialization)
@doronbehar doronbehar mentioned this pull request Oct 5, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@mhvk mhvk mhvk approved these changes

Assignees

No one assigned

Labels

no-changelog-entry-needed Refactoring testing utils.masked

Projects

None yet

Milestone

v5.3.4

Development

Successfully merging this pull request may close these issues.

test_masked.py::TestMaskedQuantityInitialization::test_masked_quantity_getting can fail with KeyError

3 participants

@doronbehar @mhvk @pllim