API: Make fields property immutable

[pllim]

This is a follow-up of:

• API/BUG: only output user-requested columns in votable.parse #15959 that addressed columns parameter not working as expected in parse_single_table/parse #14943

#15959 (comment)

As suggested by @tomdonaldson , "... a direct API change like making the fields property immutable (e.g., making it a sequence). Since there is no documentation mentioning the impacts of modifying fields or providing a recommendation on how modify it, starting to assume the only modifications would be through add_field() would be an indirect API change. More importantly, it would be a little presumptuous and unrealistic..."

@neutrinoceros , feel free to add details as you see fit.

Thanks, all!

[neutrinoceros]

Thank you for opening this. Let me try to add a bit of context:
astropy.io.votable.tree.TableElement provides an API for adding fields at runtime (namely the add_field method), however we also have tests that bypass this API and directly extend/append to the fields attribute. In #15959, and in order to implement #14943, I needed to create an explicit distinction between all available fields (all_fields) and selected fields (fields). These lists need to be kept in sync, which it made needlessly complicated if we continue to allow users to go over add_field and creates a bit of complexity that we'd like to eventually get rid of. For instance

astropy/astropy/io/votable/tree.py

Lines 2541 to 2543 in 4ab38bc

	for field in self._fields:
	if field not in self._all_fields:
	self._all_fields.append(field)

astropy/astropy/io/votable/tree.py

Lines 2610 to 2615 in 4ab38bc

	new_fields = HomogeneousList(
	Field,
	values=[f for i, f in enumerate(self.all_fields) if i in colnumbers],
	)
	if new_fields != self._fields:
	self._fields = new_fields

This issue would be fixed by:

• deprecating external usage of .append, .extend, and other mutating methods for the fields and all_fields attributes (which I think can be accomplished by overriding these methods in a subclass)
• updating all tests that currently rely on this behavior.
• opening another (last) issue to remind ourselves of everything that can be cleaned-up when the deprecation cycle ends.

[neutrinoceros]

I have a working proof of concept but the margin's too thin I had to run. I'll open a PR later today or sometimes in the weekend. As I suspected, getting everything right quickly gets quite involved.

[neutrinoceros]

Following discussion with @eerovaher in #16316, let me reformulate the current plan for the the deprecation cycle started in that PR:

• phase 1 (as astropy 6.1): TableElement.fields is a publicly mutable list
• phase 2 (DEPR: deprecate direct mutations of TableElement.fields by making it an owned list #16316, aimed at astropy 7.0): TableElement.fields remains publicly mutable, but usage of mutating list APIs emits warnings
• phase 3 (end of cycle, astropy N.0 where N>7): the private attribute, TableElement._fields, stays a list, but its corresponding, public-facing property TableElement.fields should now return an (immutable) tuple view of it.

This plan allows the, arguably awkward, _OwnedMixin class I created in #16316 to be removed at the end of the deprecation cycle.

[pllim]

Let's see what our stakeholders (e.g., PyVO) think about this plan. Thanks!

[neutrinoceros]

Who should we ping ?

[pllim]

I asked over at #pyvo channel on Astropy Slack. Feel free to ping others you think might be interested. 🤞

[tomdonaldson]

This plan seems reasonable to me. I wish we could be more confident nobody was relying on the mutability of fields so that this could be avoided, but I appreciate the extra care involved.