DEPR: deprecate direct mutations of `TableElement.fields` by making it an owned list by neutrinoceros · Pull Request #16316 · astropy/astropy

neutrinoceros · 2024-04-20T09:01:22Z

Description

The deprecation process is quite involved since we need to raise a warning for external usage of, say, table.fields.append, but still allow this operation to be performed silently internally.
My solution is to make TableElement.fields a "owned list" (my wording), that is, a list that has an owner object (namely the TableElement instance it belongs to), which is the only one allowed to directly mutate it (using a private keyword argument _owned=True). Mutations from other actors are still allowed as a temporary measure, but now raise a DeprecationWarning.

The commit history is meant to allow reviewing my process step by step. I'll also add some comments to the diff if I can help anticipate some questions from reviewers.

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.

TODO

comment diff
handle magic methods too (__iadd__, __imul__, __setitem__ and __delitem__)
Add docstrings to _OwnedList and _OwnedHomegeneousList (inheritance order matters !)
open follow up issue to keep track of what to do when the deprecation ends (or we could just keep API: Make fields property immutable #16314 opened while the deprecation is on-going ?)

github-actions · 2024-04-20T09:01:47Z

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.

github-actions · 2024-04-20T09:01:50Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

neutrinoceros · 2024-04-20T09:45:48Z

+        return super().__setitem__(key, value)
+
+    @_locked_mutator
+    def __delitem__(self, key: SupportsIndex | slice) -> None:


A note on type annotations: I didn't intend to add them, but got them for free from my editor's supercharged auto complete.

neutrinoceros · 2024-04-20T09:47:18Z

+    return inner
+
+
+class _OwnedList(list):


It's not intended as a new feature, which is why I'm making it private by default, but I could just as easily make this class public, or move it to a different module, closer to where it's used.

neutrinoceros · 2024-04-20T09:49:31Z



-def table_from_scratch():
+def test_direct_fields_mutation():


here the diff looks fishy because it's collapsing 2 independent changes

I added a new test whose preamble is based on table_from_scratch

I removed table_from_scratch when I found that it wasn't used anywhere

I could split this cleanup into its own PR if desired.

neutrinoceros · 2024-04-20T09:55:19Z


    def add_field(self, field: Field) -> None:
-        self.fields.append(field)
-        self.all_fields.append(field)


I found that this line was not needed and in fact caused some bugs that are revealed by existing tests once we start using add_field instead of direct mutations. This is a regression from #15959, which can easily be fixed independently in a -1/+0 separate PR if desired.

eerovaher · 2024-04-22T17:35:00Z

Having a subclass that must disable a large number of its inherited methods doesn't look right. Have you considered implementing HomogeneousTuple instead?

neutrinoceros · 2024-04-22T17:39:35Z

Unfortunately mutability is still needed internally, so I don't think a tuple would suitably replace the existing data structure.

eerovaher · 2024-04-22T17:46:09Z

I'm not sure what the best solution here would be, but disabling this many methods makes it apparent that you don't want this to be a list, so why would you inherit from it?

neutrinoceros · 2024-04-22T17:49:06Z

I still want it to be a list, I just don't want it mutated by anyone who's not the container object.

eerovaher · 2024-04-22T17:51:39Z

I still want it to be a list, I just don't want it mutated...

So you don't want it to be a list.

neutrinoceros · 2024-04-22T18:14:20Z

Well to take a different angle, we're talking about an attribute that's both a list (mutable) and a property (read-only), which one could argue is already a contradiction, but that ship sailed long ago. I don't think however that it's fundamentally contradictory, the language just doesn't offer a simple way to build such properties.

eerovaher · 2024-04-22T18:23:01Z

Couldn't you have a private list and have the users access its contents through a public property that converts the list to a tuple? If the list is private then there is no need to deprecate any of its methods.

neutrinoceros · 2024-04-22T18:26:54Z

Whatever we do needs to go through a gradual deprecation process. We can't make public API private or broken in one go.

eerovaher · 2024-04-22T18:38:27Z

A vanilla tuple wouldn't be appropriate immediately, it would be necessary to implement some weird subclass of tuple that would be aware of the private list it is representing and it should also have list methods that would emit deprecation warnings. That does not sound more appealing than the weird list subclass you are creating here. The difference is that such a tuple subclass could be removed when the deprecation period is over, whereas your _OwnedList is something we would be stuck with.

neutrinoceros · 2024-04-23T07:14:57Z

Couldn't you have a private list and have the users access its contents through a public property that converts the list to a tuple?

The difference is that such a tuple subclass could be removed when the deprecation period is over

I think I see your point. However it doesn't feel right to me that the proposed intermediate step involves a "mutable tuple" of sorts. I also think such a class would end up looking a lot like _OwnedList. Actually, now that I think about it, wouldn't it make sense to keep _OwnedList as the intermediate state and then implement your proposed final state when we complete the deprecation cycle ? The one downside I see with this approach is that the public attribute would move from being a list to a tuple over a single commit, but that's also true with your proposal taken as a whole, and the alternative (having an intermediate state where it's both) seems worse (?).

eerovaher · 2024-04-24T16:11:30Z

The important thing for me is that we don't end up with a permanent immutable list subclass. Implementing something strange for the transition period can be acceptable as long as it is temporary and I don't really care about details beyond that.

pllim · 2025-08-08T13:12:42Z

Not if you come back and remove the label later after a rebase. Excessive use of keep-open label defeats the purpose of the stale bot.

neutrinoceros · 2025-08-08T14:18:45Z

I'm much more expensive than the bot though, so, at which point giving it priority is just creating busy work that could then be frowned upon even more ? 🙃

github-actions · 2026-05-06T06:22:37Z

Hi humans 👋 - this pull request hasn't had any new commits for approximately 1 year, 2 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

github-actions · 2026-10-04T07:25:42Z

Hi humans 👋 - this pull request hasn't had any new commits for approximately 5 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

github-actions · 2026-10-16T05:41:45Z

Hi humans 👋 - this pull request hasn't had any new commits for approximately 5 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

…an owned list

…lay out

github-actions Bot added the io.votable label Apr 20, 2024

github-actions Bot added the utils label Apr 20, 2024

neutrinoceros force-pushed the io/votable/depr/direct_mutations_TableElement_data branch from 1421f5f to fef1d87 Compare April 20, 2024 09:01

neutrinoceros changed the title ~~DEPR: depracte direct mutations of TableElement.fields by making it an owned list~~ DEPR: deprecate direct mutations of TableElement.fields by making it an owned list Apr 20, 2024

neutrinoceros force-pushed the io/votable/depr/direct_mutations_TableElement_data branch from fef1d87 to 27d3bca Compare April 20, 2024 09:02

neutrinoceros changed the title ~~DEPR: deprecate direct mutations of TableElement.fields by making it an owned list~~ DEPR: deprecate direct mutations of TableElement.fields by making it an owned list Apr 20, 2024

neutrinoceros force-pushed the io/votable/depr/direct_mutations_TableElement_data branch 2 times, most recently from 464abe7 to 171dcc5 Compare April 20, 2024 09:43

neutrinoceros commented Apr 20, 2024

View reviewed changes

neutrinoceros force-pushed the io/votable/depr/direct_mutations_TableElement_data branch 3 times, most recently from d4716b1 to 990c546 Compare April 20, 2024 13:58

neutrinoceros marked this pull request as ready for review April 20, 2024 14:28

neutrinoceros force-pushed the io/votable/depr/direct_mutations_TableElement_data branch 2 times, most recently from d672492 to dae25d7 Compare April 21, 2024 07:25

pllim added this to the v7.0.0 milestone Apr 22, 2024

pllim added the API change PRs and issues that change an existing API, possibly requiring a deprecation period label Apr 22, 2024

astrofrog modified the milestones: v7.2.0, v8.0.0 Nov 3, 2025

astrofrog modified the milestones: v8.0.0, v8.1.0 May 5, 2026

bsipocz removed the keep-open label May 5, 2026

github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label May 5, 2026

neutrinoceros force-pushed the io/votable/depr/direct_mutations_TableElement_data branch from 9eec9e1 to acda335 Compare May 6, 2026 08:46

pllim removed the Close? Tell stale bot that this issue/PR is stale label May 6, 2026

github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label Oct 4, 2026

neutrinoceros added keep-open and removed Close? Tell stale bot that this issue/PR is stale labels Oct 5, 2026

pllim removed the keep-open label Oct 14, 2026

github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label Oct 15, 2026

neutrinoceros added 10 commits October 16, 2026 09:09

DEPR: implement _OwnedList and _OwnedHomogeneousList

51f69e1

TST: move away from to-be-deprecated API

a79aeae

BUG: fix a double-registration in TableElement.add_field

4526dd7

TST: add tests for incoming deprecation

4a49c34

DEPR: deprecate direct mutations of TableElement.fields by making it …

29723ba

…an owned list

CLN: cleanup unused test function

57c6929

DOC: add changelog entry

8b8db29

DOC: add details to _OwnedList's docstring

bdebdca

RFC: refactor _OwnedMixin and clarify how end of deprecation should p…

68e6765

…lay out

rewrite deprecation messages with 7.1 as the target

80ca22c

neutrinoceros force-pushed the io/votable/depr/direct_mutations_TableElement_data branch from acda335 to 80ca22c Compare October 16, 2026 07:11

pllim removed the Close? Tell stale bot that this issue/PR is stale label Oct 16, 2026

Conversation

neutrinoceros commented Apr 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions Bot commented Apr 20, 2024

Uh oh!

github-actions Bot commented Apr 20, 2024

Uh oh!

neutrinoceros Apr 20, 2024

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Apr 20, 2024

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Apr 20, 2024

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Apr 20, 2024

Choose a reason for hiding this comment

Uh oh!

eerovaher commented Apr 22, 2024

Uh oh!

neutrinoceros commented Apr 22, 2024

Uh oh!

eerovaher commented Apr 22, 2024

Uh oh!

neutrinoceros commented Apr 22, 2024

Uh oh!

eerovaher commented Apr 22, 2024

Uh oh!

neutrinoceros commented Apr 22, 2024

Uh oh!

eerovaher commented Apr 22, 2024

Uh oh!

neutrinoceros commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eerovaher commented Apr 22, 2024

Uh oh!

neutrinoceros commented Apr 23, 2024

Uh oh!

eerovaher commented Apr 24, 2024

Uh oh!

pllim commented Aug 8, 2025

Uh oh!

neutrinoceros commented Aug 8, 2025

Uh oh!

github-actions Bot commented May 6, 2026

Uh oh!

github-actions Bot commented Oct 4, 2026

Uh oh!

github-actions Bot commented Oct 16, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

neutrinoceros commented Apr 20, 2024 •

edited

Loading

neutrinoceros commented Apr 22, 2024 •

edited

Loading