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

Fix building distribution of PEP 561 stub-only packages #2000

Merged
merged 6 commits into from
Apr 22, 2020

Conversation

hoefling
Copy link
Contributor

@hoefling hoefling commented Feb 8, 2020

Pull Request Check List

  • Added tests for changed code.
  • Updated documentation for changed code no documentation updates needed, this patch only fixes what has worked before.

This is a fix for a regression in #475, first introduced in c401850. Added explicit check for PEP 561 stub-only packages, put duplicate code in a separate method. To avoid the regression reintroduced, added tests for:

  • stub-only wheels,
  • stub-only source dists,
  • stub-only package name checking.

Thank you in advance for a review!

@finswimmer finswimmer requested a review from a team February 8, 2020 06:57
@hoefling hoefling removed the request for review from a team March 2, 2020 18:46
@hoefling
Copy link
Contributor Author

hoefling commented Mar 2, 2020

@finswimmer I have accidentally removed the review request you have added - can you add it again?

@finswimmer finswimmer requested a review from a team March 2, 2020 19:16
laurentS added a commit to laurentS/neomodel-stubs that referenced this pull request Apr 3, 2020
There is a bug in poetry, which should be fixed with
python-poetry/poetry#2000
but until then, having this extra file seems to allow
installation.
Copy link
Member

@abn abn 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, can you please rebase this PR?

Signed-off-by: oleg.hoefling <[email protected]>
@hoefling
Copy link
Contributor Author

@abn sure thing, done!

Copy link
Member

@abn abn left a comment

Choose a reason for hiding this comment

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

Minor suggestions. Sorry, should have picked it up in the last round.

It would be good to not treat them as properties because they, can be expensive calls depending on the number of elements.

poetry/masonry/utils/package_include.py Outdated Show resolved Hide resolved
poetry/masonry/utils/package_include.py Outdated Show resolved Hide resolved
@hoefling
Copy link
Contributor Author

@abn no worries, better apply changes now than deal with bad design later. I guess I was too distracted by package and source properties, but should look at is_package/is_module methods instead.

@abn abn merged commit 9a28396 into python-poetry:master Apr 22, 2020
@abn
Copy link
Member

abn commented Apr 22, 2020

@hoefling thank you for the fix; feel free to provide a port of the fix to python-poetry/core repo too if you can. Otherwise, I can do a port sometime later this week.

hoefling added a commit to hoefling/core that referenced this pull request Apr 22, 2020
Signed-off-by: oleg.hoefling <[email protected]>
@hoefling
Copy link
Contributor Author

@abn gladly, was actually easier than I thought. python-poetry/poetry-core#28

abn pushed a commit to python-poetry/poetry-core that referenced this pull request Apr 22, 2020
Signed-off-by: oleg.hoefling <[email protected]>
abn added a commit to python-poetry/poetry-core that referenced this pull request Apr 22, 2020
@sdispater sdispater mentioned this pull request Jun 5, 2020
Copy link

github-actions bot commented Mar 1, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants