Skip to content

Clear node collection cache after collection is done #6491

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

Merged

Conversation

nicoddemus
Copy link
Member

Also rename the variable to convey its intent better

Copy link
Contributor

@blueyed blueyed left a comment

Choose a reason for hiding this comment

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

I think the renaming is not necessary, and if so should go with type hints then at least.

@nicoddemus
Copy link
Member Author

I think the renaming is not necessary

I find it makes the intent of the cache clearer (only meant to use during collection); unless you are against it I would like to keep it.

I will add the type hints, thanks. 👍

@nicoddemus nicoddemus force-pushed the clear-node-collection-cache branch from 284f5b6 to 7d09db9 Compare January 17, 2020 12:59
@nicoddemus nicoddemus force-pushed the clear-node-collection-cache branch from 7d09db9 to f84157e Compare January 17, 2020 19:10
@nicoddemus
Copy link
Member Author

Done, thanks for the pointers!

Had to add a few more type hints and also applied the same logic to the old _pkg_roots attribute.

I want to eventually refactor the entire logic out of the Session class so the session instance is not so polluted with temporary variables/caches.

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

Looks like a good change to me.

Is this meant to fix a correctness issue or just optimization/reduce memory usage?

Also rename the involved variables to convey its intent better and
add type hints
@nicoddemus nicoddemus force-pushed the clear-node-collection-cache branch from f84157e to 7b1e3d1 Compare January 21, 2020 10:29
@nicoddemus nicoddemus changed the base branch from master to features January 21, 2020 10:29
@nicoddemus
Copy link
Member Author

Is this meant to fix a correctness issue or just optimization/reduce memory usage?

The latter. 😁

@nicoddemus
Copy link
Member Author

Rebased this on features too, as it should be.

blueyed added a commit to blueyed/pytest that referenced this pull request Jan 21, 2020
blueyed added a commit to blueyed/pytest that referenced this pull request Jan 21, 2020
@nicoddemus nicoddemus merged commit e17f5fa into pytest-dev:features Jan 22, 2020
@nicoddemus nicoddemus deleted the clear-node-collection-cache branch January 22, 2020 19:09
blueyed added a commit to blueyed/pytest that referenced this pull request Jan 22, 2020
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.

3 participants