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 bugs where VCS dev-dependencies are poetry install/show even in --no-dev #996

Closed
wants to merge 2 commits into from
Closed

Fix bugs where VCS dev-dependencies are poetry install/show even in --no-dev #996

wants to merge 2 commits into from

Conversation

nyanpasu64
Copy link

@nyanpasu64 nyanpasu64 commented Mar 28, 2019

Fixes #599.

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. (partially)
  • Updated documentation for changed code. (i think this changes code to match intended/documented behavior)

I added a "test that solver marks git dev-dependencies as dev".

I did not add a test that "solver marks dev-dependencies with lists of version conditions as dev".

  • I haven't used "lists of dependencies" in my own projects yet. (I just tried using Git dependencies today.)
  • tests/puzzle/test_solver.py doesn't hit either of 2 breakpoints in poetry.py if isinstance(constraint, list):.
  • tests/test_poetry.py test_poetry_with_multi_constraints_dependency() references directory tests/fixtures/project_with_multi_constraints_dependency and hits that above branch (but not the path with dev-dependencies). But it doesn't call solver.solve().

Where would that test go? Do I create a new directory fixture, or do I copy the general form of tests/puzzle/test_solver.py?


oops why are the tests failing? i swear it works on my machine... i think i've been living in a cozy and exclusively python 3.6 environment for too long

Do I edit VCSDependency.__init__() and put **kwargs after allows_prereleases=True? Force push?

Copy link

@drunkwcodes drunkwcodes left a comment

Choose a reason for hiding this comment

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

LGTM with one place to change.

@@ -21,7 +29,7 @@ def __init__(
self._rev = rev

super(VCSDependency, self).__init__(
name, "*", optional=optional, allows_prereleases=True
name, "*", optional=optional, **kwargs, allows_prereleases=True

Choose a reason for hiding this comment

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

**kwargs should be the last one.

Copy link
Author

Choose a reason for hiding this comment

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

Force push, or append a commit to change this?

Choose a reason for hiding this comment

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

It's up to you and I'm wondering where would this thread goes after force push.

@nyanpasu64
Copy link
Author

i'm pretty sure i force pushed, why is appveyor still failing?

@drunkwcodes
Copy link

i'm pretty sure i force pushed, why is appveyor still failing?

It's exciting.

@nyanpasu64
Copy link
Author

I did not add a test that "solver marks dev-dependencies with lists of version conditions as dev".

is that not a problem?

@drunkwcodes
Copy link

@jimbo1qaz No. It is included with the proper name.

@nyanpasu64
Copy link
Author

Do you mean there is already a test that ensures "dependency with list of constraints" behaves properly? And what is it called? I have not tested the behavior of my change, neither manually nor by adding a unit test.

@drunkwcodes
Copy link

@jimbo1qaz I mean your test is included in because of the 'test_' prefix.

And it covers #599 properly.

@nyanpasu64
Copy link
Author

I don't think my test (or any other test in test_solver.py) covers #599 (list of multiple version constraints), with or without dev enabled (but i could be wrong about how pendulum works). My added test covers the related Git-package issue. I need to sleep and can't discuss further.

@drunkwcodes
Copy link

@jimbo1qaz Good night.

@nyanpasu64
Copy link
Author

Any updates?

@jeamland
Copy link

jeamland commented Jun 5, 2019

I just ran into something that would be fixed by this (and got as far as writing my own fix). Any chance this will be merged soon?

@brycedrennan brycedrennan added the kind/bug Something isn't working as expected label Aug 17, 2019
@tsiq-rob
Copy link

I observe that this bug still exists, precluding dev dependencies sourced from git, which is a major pain. @sdispater Any chance of merging the fix?

@stale
Copy link

stale bot commented Nov 15, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 15, 2019
@stale stale bot removed the stale label Nov 16, 2019
@nyanpasu64
Copy link
Author

"lists of dev-dependencies" was fixed in develop by bb98480. In fact, #1005 was the exact same one-line change as I made, yet got merged quickly.

The changes I made to fix "VCS dependencies" are not in develop.

I rebased this branch on develop. I'm not working on Python projects at the moment, so I didn't test my rebase locally. I'm going to force-push and see if the tests pass.

@nyanpasu64 nyanpasu64 changed the title Fix bugs where VCS/lists of dev-dependencies are poetry install/show even in --no-dev Fix bugs where VCS dev-dependencies are poetry install/show even in --no-dev Nov 16, 2019
@AndrewPix
Copy link

Why this fix still can not be approved? We are facing this problem too

@AndrewPix
Copy link

@drunkwcodes @sdispater Could you review and merge this PR ?

@nyanpasu64
Copy link
Author

nyanpasu64 commented Jan 27, 2020

I have a project I created in older poetry. After removing a single package under Poetry version 1.0.0, this bug popped up again. Unfortunately the patch again doesn't apply cleanly. I don't know when I'll rebase it again.

I think I'll workaround it by removing my git dependency.

EDIT: Actually the bug appears to be fixed in Poetry 1.0.2?

@nyanpasu64
Copy link
Author

Already fixed in #1725.

@nyanpasu64 nyanpasu64 closed this Jan 27, 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
kind/bug Something isn't working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants