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

API: deprecate warning for show_in_browser #16068

Merged
merged 3 commits into from Feb 21, 2024
Merged

API: deprecate warning for show_in_browser #16068

merged 3 commits into from Feb 21, 2024

Conversation

MridulS
Copy link
Contributor

@MridulS MridulS commented Feb 19, 2024

Follow up from #15905

Being a bit bold here, let's add a pending deprecation warning to show_in_browser. By removing show_in_browser we can get rid of the JS code in astropy.

We can use #16067 to solicit feedback.

@github-actions github-actions bot added the table label Feb 19, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 19, 2024 •
edited by taldcroft

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?
  • 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.

@taldcroft
Copy link
Member

@MridulS - there is a merge conflict that needs to be resolved. Otherwise this looks good to me.

taldcroft
taldcroft approved these changes Feb 21, 2024
View reviewed changes
Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

Looks good!

@taldcroft taldcroft enabled auto-merge (squash) February 21, 2024 11:13
@taldcroft
Copy link
Member

I set this for auto-merge, but it looks like the milestone_checker is stuck. hmm.

@neutrinoceros
Copy link
Contributor

@taldcroft setting a milestone should retrigger and fix it (🤞🏻)

@taldcroft taldcroft added this to the v6.1.0 milestone Feb 21, 2024
@taldcroft taldcroft merged commit 2597e90 into astropy:main Feb 21, 2024
28 checks passed
@taldcroft
Copy link
Member

@neutrinoceros - do'oh!! thanx.

eerovaher
eerovaher reviewed Feb 21, 2024
View reviewed changes
astropy/table/table.py
@@ -1841,6 +1841,13 @@ def show_in_notebook(
html += jsv.ipynb(tableid, css=css, sort_columns=sortable_columns)
return HTML(html)

@deprecated(
"6.1",
pending=True,
Copy link
Member

Choose a reason for hiding this comment

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

Why is this not a proper deprecation?

Copy link
Member

Choose a reason for hiding this comment

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

I think the idea was to take this one a bit slower to gather user inputs.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think most user would see pending=True warning. Could you please open a follow-up issue on when you think we can remove pending=True here? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Might also want to post to astropy-dev once you have this follow-up issue. Any further discussions on a merged PR would be easily overlooked.

@pllim pllim added the API change PRs and issues that change an existing API, possibly requiring a deprecation period label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taldcroft taldcroft taldcroft approved these changes

@pllim pllim pllim left review comments

@eerovaher eerovaher eerovaher left review comments

Assignees
No one assigned
Labels
API change PRs and issues that change an existing API, possibly requiring a deprecation period table
Projects
None yet
Milestone
v6.1.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@MridulS @taldcroft @neutrinoceros @pllim @eerovaher