bpo-42128: Add __match_args__ to structseq-based classes by pablogsal · Pull Request #24732 · python/cpython
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Pablo!
Should we have a test for a structseq with unnamed fields? Asserting that os.stat_result.__match_args__ is expected should do it.
|
|
||
| Py_ssize_t i, k; | ||
| for (i = k = 0; i < n_members; ++i) { | ||
| if (desc->fields[i].name == PyStructSequence_UnnamedField) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can handle this in a better way: just exclude them from match_args. The other (much worse option IMHO) is generate some missing names for these.
Should we have a test for a structseq with unnamed fields? Asserting that
os.stat_result.__match_args__is expected should do it.
Done in 40bf3b7
Looking a bit closer... should we limit this to only VISIBLE_SIZE_TP(type) members? Since they're not displayed in the repr and can't be iterated over, I don't think there's any way to easily (as a user reading the code) map positional arguments to members like tm_zone and tm_gmtoff.
Plus, we can always extend it later if it turns out to be an unnecessary limitation.
Looking a bit closer... should we limit this to only
VISIBLE_SIZE_TP(type)members? Since they're not displayed in the repr and can't be iterated over, I don't think there's any way to easily (as a user reading the code) map positional arguments to members liketm_zoneandtm_gmtoff.Plus, we can always extend it later if it turns out to be an unnecessary limitation.
Good point! I agree with that. I have pushed a new commit with this limitation.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
The only improvement I might suggest is building up a list, then converting it to a tuple (or creating a tuple and shrinking it later with _PyTuple_Resize). That way we only need to loop once.
Looks good to me!
The only improvement I might suggest is building up a list, then converting it to a tuple (or creating a tuple and shrinking it later with
_PyTuple_Resize). That way we only need to loop once.
I considered that, but I think the code would be harder to read because appending to the list can fail on every iteration and we also need to covert the list to a tuple that can also fail. Also it will be less efficient so I decided with the current approach.
I can do the resize approach, that will be cleaner.
(or creating a tuple and shrinking it later with
_PyTuple_Resize). That way we only need to loop once.
Done