Skip to content

Backport PR #17747 on branch v7.0.x (Ensure doctests do not have side effects) - #17751

Merged
pllim merged 2 commits into
astropy:v7.0.xfrom
meeseeksmachine:auto-backport-of-pr-17747-on-v7.0.x
Feb 10, 2025
Merged

pllim merged 2 commits into
astropy:v7.0.xfrom
meeseeksmachine:auto-backport-of-pr-17747-on-v7.0.x

Conversation

@meeseeksmachine

Copy link
Copy Markdown

Backport PR #17747: Ensure doctests do not have side effects

@github-actions

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 modified the milestones: v7.1.0, v7.0.2 Feb 10, 2025
@pllim

pllim commented Feb 10, 2025

Copy link
Copy Markdown
Member

@mhvk any idea why this doctest fails in backport but not on main? 🤔

_________________________ [doctest] mixin_columns.rst __________________________
...
364 By default, the outcome of setting a column using this class is not helpful::
365 
366     >>> t = Table([np.arange(4)], names=["index"])
367     >>> t['data'] = ExampleDataClass()
UNEXPECTED EXCEPTION: NameError("name 'ArrayWrapper' is not defined")
Traceback (most recent call last):
  File "python3.11/doctest.py", line 1355, in __run
    exec(compile(example.source, filename, "single",
  File "<doctest mixin_columns.rst[33]>", line 1, in <module>
  File "astropy/table/table.py", line 2110, in __setitem__
    self.add_column(value, name=item, copy=True)
  File "astropy/table/table.py", line 2404, in add_column
    col = self._convert_data_to_col(
          ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "astropy/table/table.py", line 1323, in _convert_data_to_col
    data = handler(data)
           ^^^^^^^^^^^^^
  File "<doctest mixin_columns.rst[36]>", line 2, in handle_example_data_class
NameError: name 'ArrayWrapper' is not defined
docs/table/mixin_columns.rst:367: UnexpectedException

@pllim pllim added the Bug label Feb 10, 2025
@mhvk

mhvk commented Feb 10, 2025

Copy link
Copy Markdown
Contributor

I think we'd need to backport a previous fix to the registry or so, since the .rst file hasn't changed. But it seemed easier to just fix it here... I wonder if I should "forward-port" this regardless of the fact that it doesn't fail. Probably not?

@pllim

pllim commented Feb 10, 2025

Copy link
Copy Markdown
Member

Re: forward-port -- Hmm I dunno if the universe is going to explode or not if we forward port a backport, but can try after merge?

@pllim pllim 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.

Green now. Thanks, Marten!

@pllim
pllim merged commit 3ae5d9e into astropy:v7.0.x Feb 10, 2025
@pllim

This comment was marked as duplicate.

@pllim

pllim commented Feb 10, 2025

Copy link
Copy Markdown
Member

Let's see if anything explodes...

@meeseeksdev backport to main

meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Feb 10, 2025
… (Ensure doctests do not have side effects)
pllim added a commit that referenced this pull request Feb 10, 2025
…751-on-main

Backport PR #17751 on branch main (Backport PR #17747 on branch v7.0.x (Ensure doctests do not have side effects))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants