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.

Sign up for GitHub

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

Jump to bottom

Updating spinner API, keeping enum #11046

Merged
merged 4 commits into from
May 21, 2021
Merged

Updating spinner API, keeping enum #11046

merged 4 commits into from
May 21, 2021

Conversation

i-am-b-soto
Copy link
Contributor

@i-am-b-soto i-am-b-soto commented Nov 15, 2020 •
edited

Updating the spinner API, keeping the original enum.
See #11015 for the alternative method (removing the enumerator)

Fixes #11004

@github-actions github-actions bot added the utils label Nov 15, 2020
@i-am-b-soto i-am-b-soto mentioned this pull request Nov 15, 2020
Closed
@embray embray self-requested a review November 15, 2020 22:08
@embray embray added API change PRs and issues that change an existing API, possibly requiring a deprecation period Enhancement labels Nov 15, 2020
@embray
Copy link
Member

embray commented Nov 15, 2020

LGTM; can you please also add the change from https://github.com/astropy/astropy/pull/11015/files#diff-582a94e0a0f7293634d5bb6531b7bdf8a847ecd2a380ec691a65bac8beaff546L1040 ?

There's also the issue I raised on your other PR about the interpretation of the step argument maybe actually being buggy. If you agree with me there should be a separate issue opened for that though.

@embray embray added this to the v4.3 milestone Nov 15, 2020
@i-am-b-soto
Copy link
Contributor Author

LGTM; can you please also add the change from https://github.com/astropy/astropy/pull/11015/files#diff-582a94e0a0f7293634d5bb6531b7bdf8a847ecd2a380ec691a65bac8beaff546L1040 ?

There's also the issue I raised on your other PR about the interpretation of the step argument maybe actually being buggy. If you agree with me there should be a separate issue opened for that though.

Done!
Yeah, I just thought it would be better ettiquete to not needlessly modify code XD

@i-am-b-soto
Copy link
Contributor Author

Ah! I just read your comment in the other PR about the bug in Step!
I don't mind making that another issue. Should I create the issue and resolve it before we merge this PR?

@embray
Copy link
Member

embray commented Nov 27, 2020

@Iamsoto Sorry for the late reply. Yes, let's open a separate issue for that.

@pllim pllim mentioned this pull request Apr 28, 2021
Open
@pllim
Copy link
Member

pllim commented Apr 28, 2021

Ops, sorry that this feel through the cracks. I'll just clean this up (i.e., fix the change log) and get this in. I also opened a follow-up issue at #11656.

@pllim pllim self-assigned this Apr 28, 2021
@pllim
Copy link
Member

pllim commented Apr 28, 2021

Re-reading #11046 (comment) -- @embray , should we get this in, or move the milestone. Is #11656 blocking this PR?

@eteq
Copy link
Member

eteq commented May 3, 2021

Since this hasn't made the feature freeze cutoff and is not a bug/docfix I'm re-milestoning to 5.0

@eteq eteq modified the milestones: v4.3, v5.0 May 3, 2021
@embray
Copy link
Member

embray commented May 17, 2021

I think it would be fine to address #11656 in a follow-up. It's a nice "beginner" issue.

pllim
pllim approved these changes May 18, 2021
View reviewed changes
Copy link
Member

@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.

I don't think the failures were related.

@pllim
Copy link
Member

pllim commented May 18, 2021

@embray , I'll let you do the merge since I haven't seen any formal approval from you yet.

@embray embray merged commit a45cb61 into astropy:main May 21, 2021
@pllim
Copy link
Member

pllim commented May 21, 2021

Actually, the timeout might be real. main now consistently times out after the merge of this PR. We need a follow-up patch or revert.

@embray
Copy link
Member

embray commented May 21, 2021

Wow....hard to imagine why

@pllim
Copy link
Member

pllim commented May 21, 2021

Could be a coincidence though I restarted the build a few times and it is consistently timing out. Maybe I'll ping @astrojuanlu to make sure this is not a server-side problem...

@embray
Copy link
Member

embray commented May 21, 2021

If anything it would be due to #11690 which I also just merged.

@pllim
Copy link
Member

pllim commented May 21, 2021 •
edited

But the first incident happened with this merge, according to https://readthedocs.org/projects/astropy/builds/ -- https://readthedocs.org/projects/astropy/builds/13824939/

@embray
Copy link
Member

embray commented May 21, 2021

Yes, and RTD built that PR fine...

@embray embray mentioned this pull request May 21, 2021
Merged
@pllim pllim mentioned this pull request May 21, 2021
Closed
@embray embray mentioned this pull request May 21, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pllim pllim pllim approved these changes

@embray embray Awaiting requested review from embray

Assignees

@pllim pllim

Labels
API change PRs and issues that change an existing API, possibly requiring a deprecation period Enhancement utils
Projects
None yet
Milestone
v5.0
Development

Successfully merging this pull request may close these issues.

Outdated documentation for Spinner
4 participants
@i-am-b-soto @embray @pllim @eteq