use existing function convert_deps_to_pip, added 2 params to convert_… #5077

fraser-langton · 2022-04-23T07:06:41Z

running pipenv requirements with the below example dependency will fail

"exmple-repo": {
"editable": true,
"git": "ssh://git@bitbucket.org/code/exmaple-repo.git",
"ref": "cc858e89f19bc0dbd70983f86b811ab625dc9292"
}

Fix

Use the exisiting pipenv.utils.dependencies.convert_deps_to_pip function with some added parameters to output the requirements, remove parsing of file in pipenv.cli.command.requirements itself

The checklist

Associated issue
A news fragment in the news/ directory to describe this fix with the extension .bugfix, .feature, .behavior, .doc. .vendor. or .trivial (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

Fixes #5076

…deps_to_pip

oz123

Can you add a small test case together with the code?

fraser-langton · 2022-04-25T03:26:58Z

Can you add a small test case together with the code?

@oz123 I'm struggling to find an instance were the pipfile.lock is mocked for a test? Any suggestions?

matteius · 2022-04-25T03:34:37Z

Can you add a small test case together with the code?

@oz123 I'm struggling to find an instance were the pipfile.lock is mocked for a test? Any suggestions?

@fraser-langton You don't really need to mock the Pipfile.lock right, you can just write out a file that is a valid Pipfile.lock and the current tests do that by doing a pipenv lock ahead of the requirements generation. All of the current tests for this command exist here: https://github.com/pypa/pipenv/blob/main/tests/integration/test_requirements.py

Alternatively you could generate a lock file locally and write that to a file at the start of your test and generate the requirements from that to test.

fraser-langton · 2022-04-25T03:55:46Z

@matteius I'm blind 🤦‍♂️ I was looking in the wrong directory. Thanks!

matteius · 2022-04-25T10:00:49Z

@fraser-langton can you merge latest main branch into your branch and resolve the merge conflict?

oz123

Thanks for adding that test case.

fraser-langton

Test case added for git requirement

oz123

All you need is to rebase the branch and you are done! Thanks for the contribution!

…quirements` (pypa#5071) * Print `-e` option and path for editable package * Reduce depth for logical complexity * Add news fragment

Fix pypa#5073

* Remove crayons usage from exceptions.py Replace all crayon color with appropriate `click.style` calls.

NamedTemporaryFile was introduced in Python 3.4. We only support Python 3.7+, hence this goes away.

Earlier versions of Python had Mapping in ``collections``. All versions after 3.4 moved Mapping to ``collections.abc`` Since we only support version 3.7+ we can drop this.

pathlib was introduced after Python3.5. We now only support Python3.7+, hence we don't need compat here.

This function is a nice to have alias for one line of code. Also, it uses OrderedDict, which is no longer needed. `dict`s after Python3.6 are guaranteed to be ordered (bonus: dict is faster than ordered dict). If we really want, we can also import it from zipp or python finder.

The former is simply a copy paste of `click.echo`. Since `click` has a much larger user base, it is likely to correct all bugs on all OSes faster.

* vendor in requirementslib==1.6.4 * add news fragment.

fraser-langton · 2022-04-25T10:21:20Z

@oz123 Think I did correctly :)

matteius · 2022-04-25T10:28:44Z

You are almost there @fraser-langton -- There is still a merge conflict that must be resolve with pipenv/cli/command.py -- Probably rebase was not the way to go but merging in the latest main, because now its showing a all the latest changes from main in this PR, which is confusing. Either way, the conflicting file remains.

oz123 · 2022-04-25T10:41:39Z

@fraser-langton you'll have to restart all over again. I am sorry...
Since we merged a PR which works on the same feature, not sure how to rebase this. You will have to look into the code again and see how to incorporate your commits.

It could be that the logic of :

 if value.get("editable", False):

Should be implemented in

pipenv/pipenv/utils/dependencies.py

Line 246 in f73e76c

    
           def convert_deps_to_pip(deps, project=None, r=True, include_index=True, include_hashes=True, include_markers=True):

That way, the PR we merged earlier can be integrated in a nicer way with yours.

matteius · 2022-04-25T11:13:51Z

@fraser-langton You may just want to make a new branch off the latest main and cherry-pick in all of your commits up until the rebase, but consider that there is the conflict to resolve and as @oz123 mentions this other change to fix editable installs for that command --
#5071

hoyaaaa · 2022-04-25T11:57:20Z

pipenv/cli/command.py

-            echo(crayons.normal("".join([req_name, value["version"], *hashes])))
+        deps.update(lockfile['develop'])
+
+    pip_deps = convert_deps_to_pip(deps, r=False, include_index=False, with_hashes=hash, with_markers=False)


I also tried like PR #5071 to resolve it, but there is a much better code. 😅
I will try my best to make a great code like you. 😎
But there is an error in this part. It needs to be fixed.

pip_deps = convert_deps_to_pip(deps, r=False, include_index=False, include_hashes=hash, include_markers=False)

@hoyaaaa what's the error?

oz123 · 2022-04-25T14:02:42Z

@fraser-langton I created a new PR to fix the conflict issue. I think this gets the original intent of your code.
Thank you for the contribution and the suggestion to use convert_deps_to_pip.
@hoyaaaa thank you too, for the original implementation.

fraser-langton · 2022-04-25T15:38:46Z

closed with #5083

Fraser Langton added 2 commits April 23, 2022 16:55

use existing function convert_deps_to_pip, added 2 params to convert_…

d7c4630

…deps_to_pip

news

170d816

fraser-langton marked this pull request as ready for review April 23, 2022 08:50

oz123 requested changes Apr 23, 2022

View reviewed changes

Fraser Langton added 2 commits April 25, 2022 19:39

use with_hash and with_markers

b535cd4

test case for requirements with git req

d35ab4c

oz123 approved these changes Apr 25, 2022

View reviewed changes

fraser-langton commented Apr 25, 2022

View reviewed changes

fraser-langton requested a review from oz123 April 25, 2022 10:04

oz123 reviewed Apr 25, 2022

View reviewed changes

change-park and others added 11 commits April 25, 2022 20:17

Print -e option and path for editable package when using `pipenv re…

a0f0fb5

…quirements` (pypa#5071) * Print `-e` option and path for editable package * Reduce depth for logical complexity * Add news fragment

Limit resource usage in CI (pypa#5079)

8c58ed0

Fix pypa#5073

Drop crayons in exceptions.py (pypa#5080)

0c8fd2a

* Remove crayons usage from exceptions.py Replace all crayon color with appropriate `click.style` calls.

Replace vistir.compat.NamedTemporaryFile with STL

ead2590

NamedTemporaryFile was introduced in Python 3.4. We only support Python 3.7+, hence this goes away.

Removed vistir.compat.Mapping for collections.abc

331b38f

Earlier versions of Python had Mapping in ``collections``. All versions after 3.4 moved Mapping to ``collections.abc`` Since we only support version 3.7+ we can drop this.

Remove pipenv.vendor.vistir.compat.Path

7348ae8

pathlib was introduced after Python3.5. We now only support Python3.7+, hence we don't need compat here.

Replace vistir.misc.echo with click.secho

2f3e1e5

The former is simply a copy paste of `click.echo`. Since `click` has a much larger user base, it is likely to correct all bugs on all OSes faster.

Add news fragment - describe dropping usage of vistir.

5afe5b1

vendor in requirementslib==1.6.4 (pypa#5082)

93ae6ae

* vendor in requirementslib==1.6.4 * add news fragment.

rebase

f73e76c

hoyaaaa reviewed Apr 25, 2022

View reviewed changes

fraser-langton closed this Apr 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use existing function convert_deps_to_pip, added 2 params to convert_… #5077

use existing function convert_deps_to_pip, added 2 params to convert_… #5077

fraser-langton commented Apr 23, 2022 •

edited

Loading

oz123 left a comment

fraser-langton commented Apr 25, 2022

matteius commented Apr 25, 2022

fraser-langton commented Apr 25, 2022

matteius commented Apr 25, 2022

oz123 left a comment

fraser-langton left a comment

oz123 left a comment

fraser-langton commented Apr 25, 2022

matteius commented Apr 25, 2022

oz123 commented Apr 25, 2022 •

edited

Loading

matteius commented Apr 25, 2022 •

edited

Loading

hoyaaaa Apr 25, 2022 •

edited

Loading

fraser-langton Apr 25, 2022

oz123 commented Apr 25, 2022

fraser-langton commented Apr 25, 2022

use existing function convert_deps_to_pip, added 2 params to convert_… #5077

use existing function convert_deps_to_pip, added 2 params to convert_… #5077

Conversation

fraser-langton commented Apr 23, 2022 • edited Loading

Fix

The checklist

oz123 left a comment

Choose a reason for hiding this comment

fraser-langton commented Apr 25, 2022

matteius commented Apr 25, 2022

fraser-langton commented Apr 25, 2022

matteius commented Apr 25, 2022

oz123 left a comment

Choose a reason for hiding this comment

fraser-langton left a comment

Choose a reason for hiding this comment

oz123 left a comment

Choose a reason for hiding this comment

fraser-langton commented Apr 25, 2022

matteius commented Apr 25, 2022

oz123 commented Apr 25, 2022 • edited Loading

matteius commented Apr 25, 2022 • edited Loading

hoyaaaa Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

fraser-langton Apr 25, 2022

Choose a reason for hiding this comment

oz123 commented Apr 25, 2022

fraser-langton commented Apr 25, 2022

fraser-langton commented Apr 23, 2022 •

edited

Loading

oz123 commented Apr 25, 2022 •

edited

Loading

matteius commented Apr 25, 2022 •

edited

Loading

hoyaaaa Apr 25, 2022 •

edited

Loading