Skip to content

BUG: Fix VOTable unified IO write and check_token behavior - #20465

Open
pllim wants to merge 4 commits into
astropy:mainfrom
pllim:naughty-xml-no-write
Open

BUG: Fix VOTable unified IO write and check_token behavior#20465
pllim wants to merge 4 commits into
astropy:mainfrom
pllim:naughty-xml-no-write

Conversation

@pllim

@pllim pllim commented Sep 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Description

This pull request is to fix small part of #20457 . Better not backport as this might break some use cases downstream, maybe.

AI Disclosure

If AI tools were used to develop this pull request, describe the tools including specific model and version, how they were used, and what content is AI generated. Otherwise enter "N/A".

N/A

  • I certify that I am human and that I take full responsibility for this pull request including all interactions with reviewers.

Merge method

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

@pllim pllim added this to the v8.1.0 milestone Sep 22, 2026
@pllim pllim added the Bug label Sep 22, 2026
@github-actions

github-actions Bot commented Sep 22, 2026

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.

@pllim
pllim marked this pull request as ready for review September 23, 2026 00:51
@pllim

This comment was marked as resolved.

Sign in to view
@jdavies-st

jdavies-st commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

@jdavies-st , I am guessing that your much more involved #20469 has superseded this PR?

No! You've corrected an issue I have not. 👏

I'll have a closer look and give a review today.

jdavies-st
jdavies-st suggested changes Sep 24, 2026
View reviewed changes

@jdavies-st jdavies-st left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @pllim! This is a good start, and it actually addresses part of issue #20457 that my own attempt at a fix in #20469 does not - having check_token actually warn the user when they are going to write ill-formed (and thus probably not portable) XML VO table.

In my PR, I did not actually do anything about xmlutil.check_token() not returning an error or warning. This does. And that part is important.

That said, I think there are 3 issues here in this PR:

  1. I think write_table_votable() is probably not the location to fix the invalid characters. It only gets called for `Table.write(format="votable"). A few cases slip through that do not pass through this function, as mentioned below. And those are in the docs as ways to do it.
  2. It adds a lot of memory overhead (temporarily 2x) and slows down the write by a factor of ~2x.
  3. We may want to warn_or_raise within check_token(). It is used on read and write, and I think following the other patterns within that file, we want to default to be silent on read (user can't do anything about it) and warn on write (user can fixup before writing).

So I would drop the connect.py changes, but otherwise, this looks good.

Comment thread astropy/io/votable/connect.py Outdated
Comment thread astropy/io/votable/xmlutil.py Outdated
Comment thread astropy/io/votable/tests/test_table.py Outdated
pllim and others added 4 commits September 25, 2026 14:43
@pllim
BUG: Fix VOTable unified IO write and check_token behavior
42b4d70
@pllim @jdavies-st
Rewrite test with expected behavior
ad9437f 
but this will fail without follow-up patch so [ci skip]

Co-authored-by: James Davies <[email protected]>
@pllim @jdavies-st
Change check_token logic
14edb67 
but need more code [ci skip]

Co-authored-by: James Davies <[email protected]>
@pllim @jdavies-st
Clean up code, make test pass
1c6e7a5 
Co-authored-by: James Davies <[email protected]>
@pllim
pllim force-pushed the naughty-xml-no-write branch from 262f9ca to 1c6e7a5 Compare September 25, 2026 19:13
@pllim

pllim commented Sep 25, 2026

Copy link
Copy Markdown
Member Author

@jdavies-st , I think I have addressed all your comments. Please re-review. Thanks!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tomdonaldson tomdonaldson Awaiting requested review from tomdonaldson
@stvoutsin stvoutsin Awaiting requested review from stvoutsin
@ManonMarchand ManonMarchand Awaiting requested review from ManonMarchand
@gilleslandais gilleslandais Awaiting requested review from gilleslandais
1 more reviewer
@jdavies-st jdavies-st jdavies-st requested changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Bug io.votable unified-io

Projects

None yet

Milestone

v8.1.0

Development

Successfully merging this pull request may close these issues.

2 participants

@pllim @jdavies-st