Skip to content

Conversation

@beachmachine
Copy link
Contributor

@beachmachine beachmachine commented Jul 13, 2024

This adds tests for the documented behaviour of pkgutil.extend_path regarding different argument types as well as for *.pkg files.

I took inspiration from the PR #12871 for the tests I've added. However, some of those tests did not seem to make sense to me, so I've ended up with the two test-cases in this PR.

@ghost
Copy link

ghost commented Jul 13, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app bedevere-app bot added tests Tests in the Lib/test dir awaiting review labels Jul 13, 2024
Comment on lines +557 to +559
self.assertEqual(extended_paths[-2], 'baz')
self.assertEqual(extended_paths[-1], '/foo/bar/baz')
Copy link
Member

Choose a reason for hiding this comment

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

Please also

  • check that extended_paths[:-2] is the same as the original path
  • test that duplicates are removed.

The documentation doesn't make it clear that comments are ignored. Could you note that in the docs?

Perhaps change:

apart from checking for duplicates, all entries ...

to:

blank lines, comments and duplicates are ignored; all other entries ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out that extend_path does happily insert duplicates in the list. To not introduce a potentially breaking change, I've adjusted the documentation accordingly. Also, the suggested check that extended_paths[:-2] equals the original path has been added.

This adds tests for the documented behaviour of `pkgutil.extend_path`
regarding different argument types as well as for `*.pkg` files.
@encukou
Copy link
Member

encukou commented Jul 14, 2024

Thanks! Just one more issue :)

For the future: please avoid force-pushing to CPython PRs; it makes them harder to review. Just add additional commits. The PR will be squashed when it's merged.

@beachmachine
Copy link
Contributor Author

Thanks! Just one more issue :)

For the future: please avoid force-pushing to CPython PRs; it makes them harder to review. Just add additional commits. The PR will be squashed when it's merged.

Thanks for the hint. I will not use force-push for future CPython PRs.

Also, the comment in the test is removed now, so this PR is ready to be reviewed again.

@encukou
Copy link
Member

encukou commented Jul 16, 2024

Thank you!

@encukou encukou merged commit 8f25321 into python:main Jul 16, 2024
@encukou encukou added awaiting review needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Jul 16, 2024
@miss-islington-app
Copy link

Thanks @beachmachine for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Thanks @beachmachine for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 16, 2024
pythonGH-121673)

This adds tests for the documented behaviour of `pkgutil.extend_path`
regarding different argument types as well as for `*.pkg` files.
(cherry picked from commit 8f25321)

Co-authored-by: Andreas Stocker <[email protected]>
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
…ythonGH-121673)

This adds tests for the documented behaviour of `pkgutil.extend_path`
regarding different argument types as well as for `*.pkg` files.
@encukou encukou added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes and removed needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Jul 18, 2024
@miss-islington-app
Copy link

Thanks @beachmachine for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Thanks @beachmachine for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 18, 2024
pythonGH-121673)

This adds tests for the documented behaviour of `pkgutil.extend_path`
regarding different argument types as well as for `*.pkg` files.
(cherry picked from commit 8f25321)

Co-authored-by: Andreas Stocker <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 18, 2024
pythonGH-121673)

This adds tests for the documented behaviour of `pkgutil.extend_path`
regarding different argument types as well as for `*.pkg` files.
(cherry picked from commit 8f25321)

Co-authored-by: Andreas Stocker <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jul 18, 2024

GH-121950 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Jul 18, 2024
@bedevere-app
Copy link

bedevere-app bot commented Jul 18, 2024

GH-121951 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jul 18, 2024
encukou pushed a commit that referenced this pull request Jul 19, 2024
…H-121951)

This adds tests for the documented behaviour of `pkgutil.extend_path`
regarding different argument types as well as for `*.pkg` files.
(cherry picked from commit 8f25321)

Co-authored-by: Andreas Stocker <[email protected]>
encukou pushed a commit that referenced this pull request Jul 19, 2024
…H-121950)

This adds tests for the documented behaviour of `pkgutil.extend_path`
regarding different argument types as well as for `*.pkg` files.
(cherry picked from commit 8f25321)

Co-authored-by: Andreas Stocker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review tests Tests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants