Skip to content

MNT: io.ascii deprecate and rename / remove arguments - #14914

Merged
hamogu merged 13 commits into
astropy:mainfrom
taldcroft:io-ascii-rename-args
Oct 26, 2023
Merged

MNT: io.ascii deprecate and rename / remove arguments#14914
hamogu merged 13 commits into
astropy:mainfrom
taldcroft:io-ascii-rename-args

Conversation

@taldcroft

@taldcroft taldcroft commented Jun 5, 2023

Copy link
Copy Markdown
Member

Description

This pull request is to formally deprecate a number of keyword arguments that have non-PEP8-compliant names and/or functionality that has been superceded by a new preferred API. In summary:

read():

  • Reader will be removed. Instead supply the equivalent format argument.
  • Inputter has been renamed to inputter_cls.
  • Outputter has been renamed to outputter_cls.

get_reader():

  • Reader has been renamed to reader_cls.
  • Inputter has been renamed to inputter_cls.
  • Outputter has been renamed to outputter_cls.

write():

  • Writer will be removed. Instead supply the equivalent format argument.

get_writer():

  • Writer has been renamed to writer_cls.

Fixes #8567

@taldcroft taldcroft added this to the v6.0 milestone Jun 5, 2023
@taldcroft
taldcroft requested a review from dhomeier as a code owner June 5, 2023 12:16
@github-actions

github-actions Bot commented Jun 5, 2023 •
edited by hamogu
Loading

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 "When to rebase and squash commits".
  • 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.

@taldcroft taldcroft mentioned this pull request Jun 5, 2023
Closed
2 tasks
@pllim pllim added Refactoring API change PRs and issues that change an existing API, possibly requiring a deprecation period labels Jun 5, 2023
pllim
pllim reviewed Jun 5, 2023
View reviewed changes
Comment thread docs/io/ascii/read.rst Resolved
@pllim

pllim commented Jun 5, 2023

Copy link
Copy Markdown
Member

Do you think this will break a lot of downstream code? I see a lot of API changes but I cannot tell what is internal use only and what isn't. We might want to consider:

  • Sending out a memo to astropy-dev about this PR.
  • Repeating that memo in Astropy Slack #general and mirror to relevant sub-channels.
  • If no objection, merge this soon so people have a chance to change this downstream early in the dev cycle.

@taldcroft

taldcroft commented Jun 5, 2023

Copy link
Copy Markdown
Member Author

It doesn't hurt to mention in this in a few places and I'll do that. However, I am not very worried about a lot of code breakage. The Reader=<reader_class> and Writer=<writer_class> interfaces are not in any actual code examples and it has been this way for many years. Likewise using the get_reader() and get_writer() functions should be rare for most users.

I think that part of why this looks so impacting is that a lot of the tests were written prior to updating the interface (to use format instead of Reader/Writer). And of course a lot of the internal implementation code uses these functions resulting in a lot of diffs.

@pllim

pllim commented Jun 5, 2023

Copy link
Copy Markdown
Member

specutils, for one, registers their own reader/writer at https://github.com/astropy/specutils/blob/main/specutils/io/default_loaders/ascii.py though at a glance look like they will be okay?

@taldcroft

taldcroft commented Jun 5, 2023

Copy link
Copy Markdown
Member Author

specutils, for one, registers their own reader/writer at https://github.com/astropy/specutils/blob/main/specutils/io/default_loaders/ascii.py though at a glance look like they will be okay?

Yup, it reads a table using Table.read(.., format=...), which is the desired API and is unaffected.

@taldcroft
taldcroft requested a review from hamogu June 7, 2023 10:22
hamogu
hamogu requested changes Jun 7, 2023
View reviewed changes
Comment thread astropy/io/ascii/tests/test_read.py Outdated
assert any(warn.message.args[0].startswith(msg) for warn in warns)

check_warns(ascii.read, ["a b", "1 2"])
check_warns(ascii.get_reader)

@hamogu hamogu Jun 7, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should be add a check that it still reads the table though? This test only checks the warning message. It would pass if the warning is emitted, but the output is total garbage.

I don't think we need to repeat the entire test suite with the old keywords, but since they are only deprecated but are still supposed to work I would feel more comfortable knowing we have at least one test that checks that they still work. I'm worried that we'll break them in the next few weeks if we don't have a check.

I suggest to add a simple check for just one read and one write in one format here. Or maybe you did that I just missed it?

@taldcroft
taldcroft force-pushed the io-ascii-rename-args branch from 89e1ef5 to 82288f6 Compare October 26, 2023 11:18
@taldcroft

taldcroft commented Oct 26, 2023

Copy link
Copy Markdown
Member Author

@hamogu - sorry for the late update, but I just noticed this PR hanging out. I rebased, fixed some issues and addressed your review comment for additional tests. It looks like tests are passing though a few combinations are still running. I think we're ready to finally deprecate those ancient asciitable names!

@taldcroft
taldcroft requested a review from hamogu October 26, 2023 13:17
hamogu
hamogu approved these changes Oct 26, 2023
View reviewed changes

@hamogu hamogu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Long live asciitable!

@hamogu
hamogu merged commit a9d4f99 into astropy:main Oct 26, 2023
@taldcroft
taldcroft deleted the io-ascii-rename-args branch October 26, 2023 14:48
@pllim

pllim commented Oct 26, 2023

Copy link
Copy Markdown
Member

What is still deprecated and need to be removed in 7.0 or later?

@taldcroft

taldcroft commented Oct 26, 2023

Copy link
Copy Markdown
Member Author

@pllim - If I understand the question, in the description there are 8 function kwargs that are now formally deprecated and they should all be removed in 7.0.

@pllim

pllim commented Oct 26, 2023

Copy link
Copy Markdown
Member

@taldcroft , can you please open a follow up issue and list the 8 things slated for removal? Hard to tell from all the diff here. Thanks!

@taldcroft taldcroft mentioned this pull request Oct 26, 2023
Closed
@taldcroft

taldcroft commented Oct 26, 2023

Copy link
Copy Markdown
Member Author

@pllim will #15536 do the trick so we remember? Or maybe I need a calendar reminder for a year from now?

@pllim

pllim commented Oct 26, 2023

Copy link
Copy Markdown
Member

Haha, calendar reminder is not necessary. Thanks for opening the issue! 😸

@MridulS MridulS mentioned this pull request Dec 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim left review comments
@hamogu hamogu hamogu approved these changes
@dhomeier dhomeier Awaiting requested review from dhomeier dhomeier is a code owner

Assignees

No one assigned

Labels

API change PRs and issues that change an existing API, possibly requiring a deprecation period io.ascii Refactoring

Projects

None yet

Milestone

v6.0.0

Development

Successfully merging this pull request may close these issues.

Remove deprecated io.ascii Reader/Writer and unified I/O interfaces

3 participants

@taldcroft @pllim @hamogu