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

nodes: add Node.iterchain() function #11801

Merged
merged 1 commit into from Jan 12, 2024
Merged

Conversation

bluetech
Copy link
Member

This is a useful addition to the existing listchain. While listchain returns top-to-bottom, iterchain is bottom-to-top and doesn't require an internal full iteration + reverse.

Follow up on #11785 (comment)

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, the only wart in my opinion is that the names are nearly identical, in fact I would say the only difference between them based on the names only, is that one returns a list, other an iterator, but the same contents. I'm sure I won't remember which one returns what and would need to look it up every time.

If we were designing this methods now, I would suggest iter_upwards and iter_downwards, both returning iterators. Perhaps it is worth to introduce these two methods, and keep listchain implemented as list(self.iter_downwards()) just for backward compatibility?

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it leaves a bit of a sour taste that listchain and iterchain have different order,
but its hard to name those
perhaps we could call it iterparents() but thats also not the nicest name


def listchain(self) -> List["Node"]:
"""Return a list of all parent collectors up to and including self,
starting from the root of the collection tree."""
chain = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while at it maybe also add

Suggested change
chain = []
chain = [*self.iterchain()]
chain.reverse()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my machine (Python 3.11), for a length 7 chain, the existing implementation takes ~0.4 microseconds and reusing iterchain takes ~0.7 microseconds.

I vaguely recall listchain showing up in profiles so I decided to keep the faster implementation. If you think it's not worth it, I can change it as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, in that case let's skip until we get better profiles

Thanks for the detail

@bluetech
Copy link
Member Author

If we were designing this methods now, I would suggest iter_upwards and iter_downwards, both returning iterators.

About iter_downwards, the implementation logically requires building a list, and it seems wrong to return an Iterator when you have a list. See e.g. code in SetupState which relies on the order and does slicing on the result. So I think it's good that listchain returns a list, but then iter_downwards is not a good name for it...

@nicoddemus
Copy link
Member

the implementation logically requires building a list

Ahh of course.

But the point about iterchain and listchain being identical stands, however I don't have a good suggestion, so I will leave for you to make the final decision. 👍

@bluetech
Copy link
Member Author

Options I thought of:

  1. parents property like pathlib.Path.parents.

    • Rejected because parents doesn't include self, also it has a more elaborate type than just Iterator so the analogy breaks down
  2. Add a upwards: bool = False parameter to iterchain, making the default direction same as listchain. Add a typing overload that when upwards=False, returns a list not an Iterator.

    • Rejected because iterchain is not a good name for returning a list. And I can't come up with a better name.
  3. Add upwards: bool = False to listchain, don't add a new function. Add overloads to maintain Iterator type for upwards direction.

    • Rejected because of the same issue with the name.

I do however think that 3 is somewhat preferable to the listchain/iterchain confusion, so maybe go with that?

@nicoddemus
Copy link
Member

nicoddemus commented Jan 11, 2024

I do however think that 3 is somewhat preferable to the listchain/iterchain confusion, so maybe go with that?

I agree.

We might also decide to always return an Iterator from listchain, even though this breaks backward compatibility.

This is a useful addition to the existing `listchain`. While `listchain`
returns top-to-bottom, `iterparents` is bottom-to-top and doesn't require
an internal full iteration + `reverse`.
@bluetech
Copy link
Member Author

I tried 3 and also 2 and they both end up too complex with the overloads and generator/list mixing, I don't like them.

So I kept the separate function as is, but changes its name to iterparents, hopefully that's different and suggestive enough from listchain to make it clear the order is reversed.

I also found another place to use it that I had missed before (get_scope_package).

@bluetech bluetech merged commit 5645fa4 into pytest-dev:main Jan 12, 2024
24 checks passed
@bluetech bluetech deleted the node-iterchain branch January 12, 2024 09:01
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

3 participants