MNT: io.ascii deprecate and rename / remove arguments

[taldcroft]

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

[github-actions]

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.

[pllim]

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]

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]

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]

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.

[hamogu]

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]

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

[hamogu]

Long live asciitable!

[pllim]

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

[taldcroft]

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

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

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

[pllim]

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