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

create valid PEP508 string for git dependencies with ssh (#1697) #1799

Merged
merged 1 commit into from
Jan 5, 2020

Conversation

finswimmer
Copy link
Member

Fixes: #1697

Pull Request Check List

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

  • Added tests for changed code.
  • Updated documentation for changed code.

@finswimmer finswimmer requested a review from a team December 28, 2019 08:35
@finswimmer finswimmer added kind/bug Something isn't working as expected area/build-system Related to PEP 517 packaging (see poetry-core) labels Dec 28, 2019
@@ -1,3 +1,5 @@
from poetry.vcs import git
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from poetry.vcs import git
from ...vcs import git

Copy link
Member Author

Choose a reason for hiding this comment

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

Hello @matemax,

I'm not a big fan of relative imports. "Explicit is better than implicit." and is better to read. Absolute imports are also used (most if the time) in the poetry code.

Copy link
Contributor

@matemax matemax Jan 5, 2020

Choose a reason for hiding this comment

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

Hello @finswimmer ,
It is a suggestion. I try use these code for me and noticed relative and not relative import in one file.

Copy link
Member

Choose a reason for hiding this comment

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

The rule I have personally is if the import comes from the same module/package, it should be a relative import otherwise it should be an absolute import. An example of this is https://github.com/python-poetry/poetry/blob/master/poetry/masonry/builders/builder.py

Copy link
Member

@sdispater sdispater left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@sdispater sdispater merged commit ce9ea77 into python-poetry:master Jan 5, 2020
@finswimmer finswimmer deleted the issue-01697-git-ssh branch April 4, 2020 19:05
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
area/build-system Related to PEP 517 packaging (see poetry-core) kind/bug Something isn't working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Github SSH String and SdistBuilder Incompatible
3 participants