Skip to content

DEPR: add a pending deprecation to Table.pformat's default values on arguments max_lines and max_width - #17179

Closed
neutrinoceros wants to merge 6 commits into
astropy:mainfrom
neutrinoceros:table/depr/deprecate_pformat_default_sizes
Closed

DEPR: add a pending deprecation to Table.pformat's default values on arguments max_lines and max_width#17179
neutrinoceros wants to merge 6 commits into
astropy:mainfrom
neutrinoceros:table/depr/deprecate_pformat_default_sizes

Conversation

@neutrinoceros

@neutrinoceros neutrinoceros commented Oct 15, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Fixes #15828

A couple keys for reviewers (I'll generally refer to max_lines and max_width as "max sizes"):

  • I took the initiative to also apply this deprecation to BaseColumn.pformat for consistency, but this has not been discussed on the issue. Hopefully this is also not controversial.
  • evidently, many tests needed adapting. I devised them in two categories:
    • tests that check for an exact output: I added the explicit future max sizes (-1) there to ensure stability. These can be cleaned up in the future, after the deprecation is terminated, but they do not need to be
    • tests that check for equality between two pformat return values: there I thought that leaving the implicit defaults made more sense, so I defined a unique decorator to ignore the associated warnings in these cases. This decorator should be removed when the deprecation period is terminated.
    • some tests fall contain a mix of both categories, and were decorated to ignore the warning but only after I added explicit sizes where due.

If this approach is consensual, I'll open a reminder issue to link back here for when we end the deprecation period.
Note that the branch is also written be be reviewable at a commit-by-commit level.

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

TODO:

  • pass CI
  • add test for the deprecation warnings themselves

@github-actions

github-actions Bot commented Oct 15, 2024

Copy link
Copy Markdown
Contributor

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.

@github-actions

github-actions Bot commented Oct 15, 2024

Copy link
Copy Markdown
Contributor

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@neutrinoceros neutrinoceros added this to the v7.0.0 milestone Oct 15, 2024
@neutrinoceros
neutrinoceros force-pushed the table/depr/deprecate_pformat_default_sizes branch from f5c917e to 16311cb Compare October 15, 2024 08:53
@neutrinoceros neutrinoceros mentioned this pull request Oct 15, 2024
Closed
@neutrinoceros neutrinoceros changed the title DEPR: add a pending deprecation to Table.pformat's default values on arguments max_lines and max_width DEPR: add a pending deprecation to Table.pformat's default values on arguments max_lines and max_width Oct 15, 2024
@neutrinoceros
neutrinoceros force-pushed the table/depr/deprecate_pformat_default_sizes branch from 16311cb to bfe7724 Compare October 15, 2024 09:06
@neutrinoceros neutrinoceros mentioned this pull request Oct 15, 2024
Merged
1 task
@neutrinoceros

neutrinoceros commented Oct 15, 2024

Copy link
Copy Markdown
Contributor Author

Whoops, this unearthed a couple broken test cases, fixed in #17180

@neutrinoceros
neutrinoceros force-pushed the table/depr/deprecate_pformat_default_sizes branch from bfe7724 to b182923 Compare October 15, 2024 10:19
@neutrinoceros

neutrinoceros commented Oct 15, 2024

Copy link
Copy Markdown
Contributor Author

rebased off of #17180

@neutrinoceros
neutrinoceros force-pushed the table/depr/deprecate_pformat_default_sizes branch from b182923 to a03f4c9 Compare October 15, 2024 14:45
@taldcroft

taldcroft commented Oct 15, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

@neutrinoceros - I think this has taken a wrong turn. Maybe others can chime in, but my understanding was that we just change to the pformat size defaults to -1 and make pformat_all be pending deprecation.

This does introduce a "breaking change", but the idea was that in the strong majority of cases, the pformat_all behavior is acceptable and desired. In essence, there are very few use cases where an application should care about the console size when converting a table or column to a list of strings. This is certainly the case for all the tests that are changed in this PR.

As you can see, forcing astropy tests to explicitly specify (max_lines=-1, max_width=-1) makes a huge footprint. This will likely apply to many downstream packages that have similar tests. This will also leave a bit of mess that would need to be cleaned up later, reverting all the pformat(max_lines=-1, max_width=-1) back to pformat() once the defaults are changed.

@neutrinoceros

neutrinoceros commented Oct 15, 2024

Copy link
Copy Markdown
Contributor Author

Oh... sorry I apparently misread something in the discussion and went down the wrong rabbit hole.
I'll open a separate PR then, this one is just too far from the actual goal. Let me keep it open for a couple hours while I tinker with the replacement though.

@neutrinoceros neutrinoceros mentioned this pull request Oct 15, 2024
Merged
1 task
@neutrinoceros

neutrinoceros commented Oct 15, 2024

Copy link
Copy Markdown
Contributor Author

Replaced by #17184

@neutrinoceros
neutrinoceros deleted the table/depr/deprecate_pformat_default_sizes branch October 15, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@taldcroft taldcroft Awaiting requested review from taldcroft taldcroft will be requested when the pull request is marked ready for review taldcroft is a code owner
@nstarman nstarman Awaiting requested review from nstarman nstarman will be requested when the pull request is marked ready for review nstarman is a code owner
@dhomeier dhomeier Awaiting requested review from dhomeier dhomeier will be requested when the pull request is marked ready for review dhomeier is a code owner

Assignees

No one assigned

Labels

io.ascii io.registry table

Projects

None yet

Milestone

v7.0.0

Development

Successfully merging this pull request may close these issues.

Consider changing the default behavior of Table.pformat() to be independent of the terminal

2 participants

@neutrinoceros @taldcroft