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

Refine how we detect namespace packages #12169

Merged
merged 18 commits into from
Apr 9, 2024

Conversation

nicoddemus
Copy link
Member

Previously we used a hand crafted approach to detect namespace packages, however we should rely on importlib to detect them for us.

Fix #12112

@nicoddemus nicoddemus force-pushed the 12112-ns-packages branch 2 times, most recently from 24566c5 to 504ceeb Compare March 30, 2024 15:18
@nicoddemus nicoddemus marked this pull request as ready for review March 30, 2024 15:29
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.

The new approach makes sense to me (as far as I understand the details). I left a couple of questions.

src/_pytest/pathlib.py Outdated Show resolved Hide resolved
src/_pytest/pathlib.py Outdated Show resolved Hide resolved
src/_pytest/pathlib.py Outdated Show resolved Hide resolved
src/_pytest/pathlib.py Outdated Show resolved Hide resolved
@nicoddemus nicoddemus requested a review from bluetech March 31, 2024 12:47
@nicoddemus nicoddemus removed the request for review from bluetech March 31, 2024 13:08
@nicoddemus

This comment was marked as outdated.

@nicoddemus nicoddemus marked this pull request as draft March 31, 2024 13:10
@nicoddemus

This comment was marked as outdated.

@nicoddemus nicoddemus requested a review from bluetech April 6, 2024 15:32
@nicoddemus nicoddemus marked this pull request as ready for review April 6, 2024 15:32
src/_pytest/pathlib.py Show resolved Hide resolved
src/_pytest/pathlib.py Outdated Show resolved Hide resolved
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 good to me!

Removed the test for standalone module because for some reason it was flaky (could not figure why).
@nicoddemus
Copy link
Member Author

Thanks @bluetech for the review.

I will leave this open for a few more days to give others a chance to review too (which would be appreciated).

This is better to enforce callers to check for it instead of ending up with '' and possibly breaking later.
@nicoddemus nicoddemus merged commit 9989063 into pytest-dev:main Apr 9, 2024
24 checks passed
@nicoddemus nicoddemus deleted the 12112-ns-packages branch April 9, 2024 16:23
@flying-sheep
Copy link
Contributor

Hi, this got marked as an improvement. Isn’t this a bugfix?

I’m asking because we’re trying to gauge how long we can expect to wait for a release containing this.

@nicoddemus
Copy link
Member Author

Good question, decided to go with "improvement" because it contains significant changes on how things work -- seemed safe to release this in the next minor release instead of a bug-fix release.

But it is not set in stone, if we think this is OK to go as a bug fix we can change that, for sure.

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.

pytest==8.1.1 Import regression in some namespace package layouts
4 participants