Skip to content

Conversation

@pablogsal
Copy link
Member

@pablogsal pablogsal commented Mar 3, 2021

Copy link
Member

@brandtbucher brandtbucher left a comment

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) {
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

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

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 March 3, 2021 21:30
Copy link
Member

@brandtbucher brandtbucher left a comment

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.

@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 March 3, 2021 22:55
@pablogsal
Copy link
Member Author

(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
@pablogsal pablogsal deleted the structseq branch March 4, 2021 00:03
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.

4 participants