Add new option: pip wheel --save-wheel-names #6377

PatrikKopkan · 2019-04-02T15:22:28Z

PR for #6340

src/pip/_internal/wheel.py

news/6340.feature

src/pip/_internal/commands/wheel.py

tests/functional/test_wheel.py

src/pip/_internal/commands/wheel.py

hroncok · 2019-04-11T12:26:56Z

Also, i consider the name of this PR not very descriptive about what it does. Namely, it should contain the name of the added option.

pradyunsg · 2019-04-12T14:46:06Z

Thanks @hroncok for reviewing this!

src/pip/_internal/commands/wheel.py

PatrikKopkan · 2019-04-18T08:14:59Z

I think that functionality is done. May someone from pypa tell me what should be next step? By the way it seems that the failed CI test is not my fault.

Build started
git clone -q --depth=50 https://github.com/pypa/pip.git C:\projects\pip
fatal: unable to access 'https://github.com/pypa/pip.git/': Could not resolve host: github.com
Command exited with code 128

PatrikKopkan · 2019-05-17T09:52:08Z

hi, would you have time to look at this ? @pradyunsg

hroncok · 2019-06-05T14:46:32Z

@pradyunsg Hey. What can we do to move this forward? I think this looks good from our POV.

hroncok · 2019-06-19T11:54:18Z

@pradyunsg Please?

cjerdonek

I added a few review comments.

news/6340.feature

src/pip/_internal/commands/wheel.py

pradyunsg · 2019-06-20T09:15:38Z

@pradyunsg Please?

Whoops! I've missed these notifications -- I'll take a look at this today evening.

pradyunsg

Thanks for this PR (and your patience) @PatrikKopkan! :) (and @hroncok for pinging me on this)

This PR seems to be failing on Windows (I have a suggestion inline to fix that) and the linter isn't super happy so please satisfy the linter. :P

This seems to be fairly complete functionally, and I don't have any major concerns there. I have made some inline comments about the details around the implementation / testing.

Please feel free to skip the comments starting with nit: if you disagree with them. I'm nitpicking there, so please don't mind me. I'd hate for those to be something that causes friction. :)

src/pip/_internal/commands/wheel.py

src/pip/_internal/wheel.py

tests/functional/test_wheel.py

tests/unit/test_wheel.py

news/6340.feature

src/pip/_internal/commands/wheel.py

tests/functional/test_wheel.py

src/pip/_internal/wheel.py

Review comments addressed.

cjerdonek · 2019-06-21T08:57:33Z

tests/unit/test_wheel.py

+class TestWheelCommand(object):
+    @patch('pip._internal.wheel.WheelBuilder')
+    @patch('pip._internal.commands.wheel.RequirementSet')
+    def test_save_wheelnames(self, mocked_rs, mocked_wb):


This test seems like it's going overboard with the mocking, etc, and as a result the test is a lot more complicated than it needs to be, IMO. Ideally, no calls to mock or patch should be needed. I would suggest changing save_wheelnames() to accept path_to_wheelnames and wheel_filenames and use the tmpdir fixture. Then instead of mocking open(), just let save_wheelnames() write a normal file, and check the file's contents at the end of the test. As for the RequirementSet object, I would see if you can pick a signature for save_wheelnames() that doesn't require mocking there, either. The body of the function only uses requirement_set.requirements.values(), so if necessary you could have the function accept just the values. Or if it's easy to construct a requirement_set.requirements object, you could have it accept requirements. My intention in suggesting a separate function was to have a function that is easy to test while also testing most of the file-writing logic.

Feedback of inexperienced contributor:
I admit that I am not familiar with philosophy of tests. My understanding of unit tests is that should check only tested function and should not rely on function calls of others function. When I saw there are directories dedicated to split the types of test(functional, unit) it misled me that the test should be done this way.

Feedback of inexperienced contributor:

@PatrikKopkan The feedback helps. :)

The approach @cjerdonek describes is usually simpler to test on since all the function's "true" dependencies are surfaced and thus the need for mocking is reduced, while making it clearer at the call site what is actually being used.

Do you think that having some prose on this approach to testing would help new contributors understand this better?

It might. Either way thanks for guiding me

pypa-bot · 2019-06-26T12:47:46Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

PatrikKopkan · 2019-09-19T11:19:57Z

I think it is ready for rereview.

pradyunsg · 2019-09-21T12:58:47Z

ISTM that the history of this PR isn't correct (eg 89f5adc is included but really shouldn't be). @PatrikKopkan could you look into that?

PatrikKopkan · 2019-09-27T14:13:20Z

@pradyunsg, Oh sorry

BrownTruck · 2019-09-29T07:30:04Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

pypa-bot · 2019-10-18T09:22:27Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

PatrikKopkan · 2019-10-18T09:56:27Z

@pradyunsg , thank you for help with merge conflict, is there something I could do?

pradyunsg · 2019-10-19T05:33:48Z

Hurrah! Thanks for following up on this @PatrikKopkan! ^>^

LGTM, and merged. :)

chrahunt · 2019-11-04T17:35:23Z

Is this sufficient to satisfy the original use case

output_dir="$(mktemp -d)"
pip wheel --wheel-dir "$output_dir" ...
find "$output_dir" -type f -print0 | xargs -0 pip install

If so that would be my preference, since it has a couple benefits over the new argument as mentioned in #6340 (comment). The code prior to this change was already a bit tangled, so any concerns we can avoid adding benefit the general improvements we're doing in this area (see #7285 (comment) for an example).

PatrikKopkan · 2019-11-14T11:11:25Z

Is this sufficient to satisfy the original use case
output_dir="$(mktemp -d)"
pip wheel --wheel-dir "$output_dir" ...
find "$output_dir" -type f -print0 | xargs -0 pip install
If so that would be my preference, since it has a couple benefits over the new argument as mentioned in #6340 (comment). The code prior to this change was already a bit tangled, so any concerns we can avoid adding benefit the general improvements we're doing in this area (see #7285 (comment) for an example).

Yes, it does. I originally thought that option would be better but arguments made me realize that it is not good as I thought.

This reverts commit bcad1b1, reversing changes made to f864903. As discussed, we should rethink the interface of this command output as part of larger CLI usability review. In the interim, the same functionality can be achieved using straightforward shell commands.

hroncok reviewed Apr 4, 2019

View reviewed changes

src/pip/_internal/wheel.py Outdated Show resolved Hide resolved

PatrikKopkan force-pushed the master branch from 82656a8 to 2e7276f Compare April 8, 2019 08:32

hroncok reviewed Apr 10, 2019

View reviewed changes

src/pip/_internal/wheel.py Outdated Show resolved Hide resolved

PatrikKopkan force-pushed the master branch 2 times, most recently from 95e8416 to 22b5de7 Compare April 10, 2019 12:23

hroncok suggested changes Apr 11, 2019

View reviewed changes

PatrikKopkan force-pushed the master branch 2 times, most recently from 3b92d57 to a849cf5 Compare April 12, 2019 16:26

PatrikKopkan changed the title ~~new option for wheel~~ new option for wheel --save-wheel-names Apr 15, 2019

hroncok reviewed Apr 15, 2019

View reviewed changes

src/pip/_internal/commands/wheel.py Outdated Show resolved Hide resolved

mcepl mentioned this pull request May 6, 2019

Implement macros for PEP 518 & PEP 517 build systems openSUSE/python-rpm-macros#30

Closed

PatrikKopkan force-pushed the master branch from 28f95ea to 8e98a66 Compare May 12, 2019 13:58

cjerdonek previously requested changes Jun 20, 2019

View reviewed changes

news/6340.feature Outdated Show resolved Hide resolved

src/pip/_internal/commands/wheel.py Outdated Show resolved Hide resolved

src/pip/_internal/commands/wheel.py Outdated Show resolved Hide resolved

PatrikKopkan force-pushed the master branch 2 times, most recently from edfa759 to f6924d2 Compare June 20, 2019 16:10

pradyunsg reviewed Jun 21, 2019

View reviewed changes

pradyunsg added C: wheel The wheel format and 'pip wheel' command S: awaiting response Waiting for a response/more information type: feature request Request for a new feature labels Jun 21, 2019

cjerdonek reviewed Jun 21, 2019

View reviewed changes

pradyunsg changed the title ~~new option for wheel --save-wheel-names~~ Add new option: pip wheel --save-wheel-names Jun 21, 2019

PatrikKopkan force-pushed the master branch from c811d5d to 17bf443 Compare September 25, 2019 11:38

Add new option: pip wheel --save-wheel-names

9a2181a

PatrikKopkan force-pushed the master branch from 17bf443 to 9a2181a Compare September 25, 2019 11:39

pradyunsg approved these changes Sep 28, 2019

View reviewed changes

BrownTruck added the needs rebase or merge PR has conflicts with current master label Sep 29, 2019

Merge branch 'master' into master

ff041cc

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Sep 30, 2019

add newline

211fd55

PatrikKopkan force-pushed the master branch from 211fd55 to 6b9fea3 Compare October 18, 2019 09:22

pypa-bot added the needs rebase or merge PR has conflicts with current master label Oct 18, 2019

PatrikKopkan force-pushed the master branch from 6b9fea3 to 211fd55 Compare October 18, 2019 09:23

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Oct 18, 2019

pradyunsg merged commit bcad1b1 into pypa:master Oct 19, 2019

chrahunt mentioned this pull request Nov 2, 2019

pip wheel should provide the name of the resulting file #6340

Open

chrahunt mentioned this pull request Dec 1, 2019

Revert "Add new option: pip wheel --save-wheel-names (#6377)" #7420

Merged

lock bot added the auto-locked Outdated issues that have been locked by automation label Dec 14, 2019

lock bot locked as resolved and limited conversation to collaborators Dec 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new option: pip wheel --save-wheel-names #6377

Add new option: pip wheel --save-wheel-names #6377

PatrikKopkan commented Apr 2, 2019

hroncok commented Apr 11, 2019

pradyunsg commented Apr 12, 2019

PatrikKopkan commented Apr 18, 2019

PatrikKopkan commented May 17, 2019

hroncok commented Jun 5, 2019

hroncok commented Jun 19, 2019

cjerdonek left a comment

pradyunsg commented Jun 20, 2019

pradyunsg left a comment •

edited

Loading

cjerdonek Jun 21, 2019 •

edited

Loading

PatrikKopkan Jun 21, 2019

pradyunsg Jun 21, 2019 •

edited

Loading

PatrikKopkan Jun 25, 2019

pypa-bot commented Jun 26, 2019

PatrikKopkan commented Sep 19, 2019

pradyunsg commented Sep 21, 2019

PatrikKopkan commented Sep 27, 2019

BrownTruck commented Sep 29, 2019

pypa-bot commented Oct 18, 2019

PatrikKopkan commented Oct 18, 2019

pradyunsg commented Oct 19, 2019

chrahunt commented Nov 4, 2019

PatrikKopkan commented Nov 14, 2019

Add new option: pip wheel --save-wheel-names #6377

Add new option: pip wheel --save-wheel-names #6377

Conversation

PatrikKopkan commented Apr 2, 2019

hroncok commented Apr 11, 2019

pradyunsg commented Apr 12, 2019

PatrikKopkan commented Apr 18, 2019

PatrikKopkan commented May 17, 2019

hroncok commented Jun 5, 2019

hroncok commented Jun 19, 2019

cjerdonek left a comment

Choose a reason for hiding this comment

pradyunsg commented Jun 20, 2019

pradyunsg left a comment • edited Loading

Choose a reason for hiding this comment

cjerdonek Jun 21, 2019 • edited Loading

Choose a reason for hiding this comment

PatrikKopkan Jun 21, 2019

Choose a reason for hiding this comment

pradyunsg Jun 21, 2019 • edited Loading

Choose a reason for hiding this comment

PatrikKopkan Jun 25, 2019

Choose a reason for hiding this comment

pypa-bot commented Jun 26, 2019

PatrikKopkan commented Sep 19, 2019

pradyunsg commented Sep 21, 2019

PatrikKopkan commented Sep 27, 2019

BrownTruck commented Sep 29, 2019

pypa-bot commented Oct 18, 2019

PatrikKopkan commented Oct 18, 2019

pradyunsg commented Oct 19, 2019

chrahunt commented Nov 4, 2019

PatrikKopkan commented Nov 14, 2019

pradyunsg left a comment •

edited

Loading

cjerdonek Jun 21, 2019 •

edited

Loading

pradyunsg Jun 21, 2019 •

edited

Loading