Optional accessors in Sources

[Pulga8]

Fixes #4221

This PR updates ListSource and TreeSource so accessors are optional when the input data is mapping-based (for example, dictionaries). Previously, accessors were effectively required for all non-Row inputs, which made mapping-based usage more restrictive than necessary.

Changes:

• Accepts accessors as optional (None defaults to an empty accessor list).
• Preserves strict validation for non-mapping data, which still requires explicit accessors.
• Applies the same behavior in both list and tree sources.
• Add test for valid and invalid scenarios.

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[corranwebster]

Thanks for the contribution!

I'm a new core team member, so I'm not going to approve or request changes directly, but I will comment.

I'm in two minds about whether "missing accessors" should be represented by [] or by None, but either way there is a mismatch in the default value vs. the value of self._accessors. Either:

• the default value in the __init__ should be [] or () or something like that (and None is not the default, nor allowed), and self._accessors is then []; or
• the default value in the __init__ should be None (and if an empty sequence is given, it is converted to None) and self._accessors is set to None

The empty list has a certain elegance, but None may be more Pythonic.

[corranwebster]

It might be nicer to have the logic look more like:

elif len(self._accessors) > 0:
    if hasattr(....) ...:
        node = Node(...)
    else:
        node = Node(...)
else:
    raise ValueError(...)

(and similarly in the ListSource).

This is not a required change.

[freakboy3742]

I'm in two minds about whether "missing accessors" should be represented by [] or by None, but either way there is a mismatch in the default value vs. the value of self._accessors. Either:

• the default value in the __init__ should be [] or () or something like that (and None is not the default, nor allowed), and self._accessors is then []; or

Not sure if I'm misunderstanding your intention here, but the value of a default argument absolutely must not be [] , because the [] will be instantiated at time of parsing. If you need to use an empty list as a default argument, you must pass None (or some other sentinel value), and process that sentinel into [] in the method.

However, in this case, I'd be inclined to say that "there are no accessors" should be represented as None, rather than "empty list", on "pythonic" grounds - there's a difference between "this list exists and is empty", and "this list does not exist"; and even though "this list is empty" isn't an interpretation that makes much sense here, the None interpretation is closer to the intention, IMHO.

[Pulga8]

I have a question about this. I used the data parameter as a reference, and it's defined in __init__ like this:

data: Iterable | None = None

Then it's initialized like this:

if data is not None:
    self._data = [self._create_row(value) for value in data]
else:
    self._data = []

So when data is None, it ends up being initialized as an empty list ([]).
Am I misunderstanding something here? Isn't this a case of type inconsistency too?

[freakboy3742]

So when data is None, it ends up being initialized as an empty list ([]). Am I misunderstanding something here? Isn't this a case of type inconsistency too?

Apologies for the confusion here. There's one more typing detail that (hopefully) clarifies what is going on here.

You've identified the type declaration of data as an argument to __init__() (L111 in your current version) - data: Iterable | None = None. That's the type of valid values when constructing a ListSource.

However, there's also a type declaration on L108: _data: list[Row]. That is the type declaration of the data as stored internally. The if data is None transformation that you've highlighted is essentially converting between the two forms - the constructor is liberal in what it accepts, but conservative in what it stores.

This is because of the detail I flagged in my previous review- we cannot use [] as a default value in the constructor. Consider the following:

value = []
def demo(arg | list = []):
    arg.append(42)
    value.extend(arg)

demo()
print(f"first {value=}")
demo()
print(f"second {value=}")

This code code reads as if the "arg" argument has a default value of [] (and None is not an allowed value). A simple reading of that code would lead you to thing that it would print "first value=[42]" then "second value=[42]" (as both calls use the "default" argument value). However, you actually get "second value=[42, 42, 42]". This is because the value of the default is evaluated once, when the function is parsed; as a result, the same list instance is used as a default every time the function is invoked. This is true for any mutable data type.

So - a general pattern if you want "default value is a list" (or any other mutable data type) is to accept a value of None, and then convert that value of None into the "actual" value that you want. In this case:

def demo(arg: list | None = None):
    if arg is None:
        arg = []
    arg.append(42)
    value.extend(arg)

So - that's how data is being handled. data must be a list when stored internally; but the default value is an "empty list", specified using a default value of None.

However - accessors is a slightly different matter. There is a conceptual difference between "there are no accessors" and "accessors have not been defined" - so it makes sense (to me), for None to be a legal value for accessors. That means the type declaration for _accessors is _accessors: list[str] | None, which matches the constructor argument; the None value would be preserved if provided.

Does that make any more sense?

[freakboy3742]

@Pulga8 Is this ready for a re-review, or are you planning to make additional changes?

[Pulga8]

Yes, it's ready. I believe I've interpreted the request correctly. Apologies if I misunderstood, and thank you for your patience.

[freakboy3742]

Thanks - this looks great. The only issue of any consequence is the exact handling of accessors = [] case; as I've noted inline, it's a simpler implementation and more internally consistent to allow an empty list, even though it's a weird edge case.

I've also tweaked the documentation and error messages; the documentation was an accurate description of what was being done in the implementation, but didn't really provide the guidance needed as a user of the API.

Thanks for the PR!

[freakboy3742]

A minor code style issue - we try to avoid the "all args on one separate line" format, even though it's legal from Ruff's perspective. We prefer to go directly to "one arg per line":

Suggested change

	self, accessors: Iterable[str] | None = None, data: Iterable | None = None
	self,
	accessors: Iterable[str] | None = None,
	data: Iterable | None = None

[freakboy3742]

Suggested change

	in each column of the row. If omitted, only mapping row data can be
	converted.
	in each column of the row. If omitted, row data must be specified as a
	mapping.

[freakboy3742]

It's an edge case; but for consistency, I'd argue an empty list should be interpreted as a list of no accessors, rather than "accessors haven't been provided". That would technically allow construction of "empty" Row objects from list-like data. That isn't really useful, but it's internally consistent (and the implementation is simpler as well).

Suggested change

	accessors = list(accessors)
	self._accessors = accessors if len(accessors) > 0 else None
	self._accessors = list(accessors)

[freakboy3742]

As with list source; this can be simplified for consistency.

[freakboy3742]

This is an accurate description of the implementation, but that doesn't describe how or why you'd use a non-accessor ListSource. The docs should be focussed on usage.

[Pulga8]

Thanks for your re-review, freakboy3742, and for the helpful feedback!