Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-42128: Add __match_args__ to structseq-based classes #24732

Merged
merged 4 commits into from Mar 4, 2021

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Mar 3, 2021

Copy link
Member

@brandtbucher brandtbucher left a comment

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) {
Copy link
Member Author

@pablogsal pablogsal Mar 3, 2021

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.

@pablogsal
Copy link
Member Author

pablogsal commented Mar 3, 2021

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

@brandtbucher
Copy link
Member

brandtbucher commented Mar 3, 2021

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.

@pablogsal
Copy link
Member Author

pablogsal commented Mar 3, 2021

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.

Good point! I agree with that. I have pushed a new commit with this limitation.

@pablogsal pablogsal requested a review from brandtbucher Mar 3, 2021
Copy link
Member

@brandtbucher brandtbucher left a comment

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.

@pablogsal
Copy link
Member Author

pablogsal commented Mar 3, 2021

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.

@pablogsal pablogsal requested a review from brandtbucher Mar 3, 2021
@pablogsal
Copy link
Member Author

pablogsal commented Mar 3, 2021

(or creating a tuple and shrinking it later with _PyTuple_Resize). That way we only need to loop once.

Done

@pablogsal pablogsal merged commit 0632b10 into python:master Mar 4, 2021
11 checks passed
@pablogsal pablogsal deleted the structseq branch Mar 4, 2021
kreathon pushed a commit to kreathon/cpython that referenced this pull request May 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants