-
-
Notifications
You must be signed in to change notification settings - Fork 28
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 an issue for projects with an at sign (@
) in the path
#310
Conversation
@
) in the path@
) in the path
Thanks @jervi -- I have a fix for the test CI failing in my PR but it requires a new version of pip-shims for my other changes. I'll be able to fix that later this week and then I'll have you merge the updated main in your branch and then the tests should be runnable. |
@jervi I have merged my changes into main -- if you can merge the latest main into your branch it should pass the next test run. |
It seems `requirementslib` is causing a crash in `pipenv` in some rare circumstances. If you define a package like this in a `Pipfile`: ``` myproject = {editable = true, path = "."} ``` and this project path contains an at sign (`@`), then requirementslib will parse the path wrong. The second clause in the added test case will fail without this fix. Fixes sarugaku#309
Thanks @matteius, I've rebased the branch now |
@jervi it looks like the one failing test is for a VCS version which is a supported use case, so I think we have to sort out how to tell if the directory includes an @ or if the end of the path is specifying a vcs repository that is specifying the checkout version. |
Hmm. I thought all the tests succeeded locally. I'll have another look. |
@jervi I think that test does not run on Windows though. |
@jervi let me know what your timeline is like; I'd like to do a release of this library in say a week or sooner if this gets finalized. My rationale for a release soon is to get the new version of pip vendored into pipenv into a release for later in April. |
Sorry, I meant to fix this during the last weekend, but things came up. I'll try to fix the test shortly - today or tomorrow 👍 |
The motivation is to only care about @ that is not part of the path but rather used for git refs
Long days these days 😆 I'm really sorry for the long delay. It's a bit cumbersome that the tests don't run automatically, so I can't really confirm, but I think it's working now. |
What is a valid git ref in this context? Is it only tags or sha's? Or can it be branch names as well? This implementation will probably break if one are using a branch name for the ref and this branch contains a slash. I might need to adjust that regex a bit more... |
Figured slashes in the ref names ought to be supported. This approach is maybe a bit naïve, but it's better than the previous attempt. All the existing tests works, and I added another test case that also works. |
I'm being hit by the bug (#309) this is trying to fix. Is there anything blocking this from being merged? |
@nnutter Just that I want more reviews and confidence that its not going to break something :-) If you think it looks good, that helps me feel better about hitting the merge button. I can try to get this into a release soon. |
It seems
requirementslib
is causing a crash inpipenv
in some rare circumstances. If you define a package like this in aPipfile
:and this project path contains an at sign (
@
), then requirementslib will parse the path wrong. The second clause in the added test case will fail without this fix.Fixes #309