Deprecate the pedantic option for VOTables by eerovaher · Pull Request #12129 · astropy/astropy

eerovaher · 2021-08-30T18:35:03Z

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

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.
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 a milestone set? Milestone must be set but astropy-bot check might be missing; 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.

The ``pedantic`` keyword argument for parsing VOTables is now deprecated together with the corresponding configuration option.

pllim · 2021-08-30T18:53:49Z

I think @bsipocz we should avoid new deprecation in LTS? So, I milestoned to 5.1, so this cannot be merged before 5.0 branching.

bsipocz · 2021-08-30T19:44:05Z

The addendum to APE-2 should be the guideline here, and I believe we said that deprecations are fine/preferred in LTS, and removal in a later version. The APE extension should preferably be wrapped up and merged though, so we have a document to consult.

bsipocz · 2021-08-30T19:47:23Z

However this is a weird case here, as verify is available for a while, so practically it doesn't really matter at this point when it will get its due DeprecationWarning.

bsipocz · 2021-08-30T19:53:18Z

        for more information. When not provided, uses the configuration setting
        ``astropy.io.votable.verify``, which defaults to 'ignore'.

-        .. versionchanged:: 4.0


Keep this note, at least until pedantic fully disappears. I would even argue that the versionadded info, without the extra explanation is a very useful thing.

Same comment as @bsipocz .

bsipocz · 2021-08-30T19:54:39Z

@@ -261,26 +260,40 @@ input.

 .. _pedantic-mode:


maybe change the ref label, too

bsipocz · 2021-08-30T19:55:17Z


 .. _pedantic-mode:

-Pedantic mode


Very nice find and cleanup, this got missed out in the review cycle of the original PR

pllim · 2021-08-30T20:04:33Z

Yes, sorry, I misremembered the policy. I moved milestone to 5.0.

Do we need to ping pyvo people to review this? If anyone will get hit by this warning, it would be pyvo, no? And maybe astroquery too.

bsipocz · 2021-08-30T20:45:54Z

A very quick look at pyvo says it will be fine, but yes, indeed astroquery will be hit. However it's not significant and in the worst case we can deal with it once this landed in astropy main (aka, the sooner the better to merge things like this before FF)

pllim · 2021-08-31T14:14:00Z

+            with pytest.raises(AstropyDeprecationWarning):
+                with pytest.raises(VOWarning):
+                    parse(get_pkg_data_filename('data/gemini.xml'))


I think this works too.

Suggested change

with pytest.raises(AstropyDeprecationWarning):

with pytest.raises(VOWarning):

parse(get_pkg_data_filename('data/gemini.xml'))

with pytest.raises(AstropyDeprecationWarning), pytest.raises(VOWarning):

parse(get_pkg_data_filename('data/gemini.xml'))

This would work, but it would also exceed the 80 character line length limit.

Our limit is 100 😸

astropy/setup.cfg

Line 153 in 46cfe3f

max-line-length = 100

I pushed the second commit before seeing your answer, and I don't think the nested with statements here are bad enough to justify making another commit just to replace them, but I can change them if anyone requests more changes or if I'm asked to squash the commits.

pllim · 2021-08-31T14:21:21Z

-`~astropy.io.votable.parse_single_table` functions::
+Many VOTable files in the wild do not conform to the VOTable specification. You
+can set what should happen when a violation is encountered with the ``verify``
+argument, which can take three values:


Suggested change

argument, which can take three values:

keyword, which can take three values:

pllim · 2021-08-31T14:25:02Z

+      same type to a maximum value using the
+      `astropy.io.votable.exceptions.conf.max_warnings
+      <astropy.io.votable.exceptions.Conf.max_warnings>` item in the
+      :ref:`astropy_config`


To be consistent.

Suggested change

:ref:`astropy_config`

:ref:`astropy_config`.

pllim · 2021-08-31T14:25:50Z

+      :ref:`astropy_config`
+    * ``'exception'`` - Do not parse the VOTable and raise an exception.
+
+The ``verify`` argument can be used with the :func:`~astropy.io.votable.parse` or


Suggested change

The ``verify`` argument can be used with the :func:`~astropy.io.votable.parse` or

The ``verify`` keyword can be used with the :func:`~astropy.io.votable.parse` or

pllim · 2021-08-31T14:27:36Z


-Even with ``pedantic`` turned off, many warnings may still be omitted.
-These warnings are all of the type
-`~astropy.io.votable.exceptions.VOTableSpecWarning` and can be turned


I see that this sentence is deleted altogether. Does this no longer apply?

This was explaining that pedantic=False can emit (not omit, that's a typo) warnings, which might have been unexpected. What the verify options do is already explained above.

eerovaher · 2021-08-31T20:57:08Z

Should I add a second commit with the implemented feedback to this PR or would it be better to force-push and replace the starting commit?

pllim · 2021-08-31T20:58:17Z

Should I add a second commit

I think second commit is fine. Thanks!

pllim

LGTM. Thanks!

As @bsipocz , let's get this in sooner than later, so downstream packages have time to adjust before feature freeze.

The `pedantic` argument for VOTable parsing was deprecated in astropy#12129, but no deprecation notice was added to the docstring. This commit fixes that oversight.

The `pedantic` option for `parse()` and `parse_single_table()` functions in `astropy.io.votable` has been deprecated since astropy/astropy#12129 in favour of the `verify` option. This commit replaces the uses of the deprecated option in `astroquery` tests.

The `pedantic` option for `parse()` and `parse_single_table()` functions in `astropy.io.votable` has been deprecated since astropy/astropy#12129 in favour of the `verify` option. This commit replaces the uses of the deprecated option in `astroquery`.

The `pedantic` option for `parse()` and `parse_single_table()` functions in `astropy.io.votable` has been deprecated since astropy/astropy#12129 in favour of the `verify` option. This commit replaces the uses of the deprecated option in `astroquery` tests.

The `pedantic` option for `parse()` and `parse_single_table()` functions in `astropy.io.votable` has been deprecated since astropy/astropy#12129 in favour of the `verify` option. This commit replaces the uses of the deprecated option in `astroquery`.

The `pedantic` option for `parse()` and `parse_single_table()` functions in `astropy.io.votable` has been deprecated since astropy/astropy#12129 in favour of the `verify` option. This commit replaces the uses of the deprecated option in `astroquery` tests.

The `pedantic` option for `parse()` and `parse_single_table()` functions in `astropy.io.votable` has been deprecated since astropy/astropy#12129 in favour of the `verify` option. This commit replaces the uses of the deprecated option in `astroquery`.

The `pedantic` option for `parse()` and `parse_single_table()` functions in `astropy.io.votable` has been deprecated since astropy/astropy#12129 in favour of the `verify` option. This commit replaces the uses of the deprecated option in `astroquery` tests.

The `pedantic` option for `parse()` and `parse_single_table()` functions in `astropy.io.votable` has been deprecated since astropy/astropy#12129 in favour of the `verify` option. This commit replaces the uses of the deprecated option in `astroquery`.

Deprecate the pedantic option for VOTables

0f2a329

The ``pedantic`` keyword argument for parsing VOTables is now deprecated together with the corresponding configuration option.

github-actions Bot added the io.votable label Aug 30, 2021

pllim added this to the v5.1 milestone Aug 30, 2021

bsipocz reviewed Aug 30, 2021

View reviewed changes

pllim modified the milestones: v5.1, v5.0 Aug 30, 2021

pllim added the Refactoring label Aug 30, 2021

pllim requested a review from tomdonaldson August 30, 2021 20:03

pllim reviewed Aug 31, 2021

View reviewed changes

Implement feedback given to astropy#12129

3596c99

pllim approved these changes Sep 1, 2021

View reviewed changes

pllim merged commit 5df77af into astropy:main Sep 1, 2021

pllim mentioned this pull request Sep 1, 2021

MNT: Remove deprecated pedantic option in VO table #12134

Closed

Cadair mentioned this pull request Sep 15, 2021

sdist & wheel builds broken #12170

Closed

eerovaher mentioned this pull request Sep 20, 2021

Add a deprecation notice to VOTable parse() doctstring #12178

Merged

9 tasks done

eerovaher mentioned this pull request Oct 4, 2021

Replace the pedantic option for VOTable parsing astropy/astroquery#2167

Merged

pllim mentioned this pull request Oct 25, 2021

Fixed CI test failures in Azure pipline #12287

Merged

9 tasks

eerovaher deleted the deprecate-pedantic branch November 8, 2021 12:27

	argument, which can take three values:
	keyword, which can take three values:

	The ``verify`` argument can be used with the :func:`~astropy.io.votable.parse` or
	The ``verify`` keyword can be used with the :func:`~astropy.io.votable.parse` or

Conversation

eerovaher commented Aug 30, 2021

Checklist for package maintainer(s)

Uh oh!

pllim commented Aug 30, 2021

Uh oh!

bsipocz commented Aug 30, 2021

Uh oh!

bsipocz commented Aug 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Aug 30, 2021

Uh oh!

bsipocz commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerovaher commented Aug 31, 2021

Uh oh!

pllim commented Aug 31, 2021

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bsipocz commented Aug 30, 2021 •

edited

Loading