Unpin commented out unsafe packages #975

atugushev · 2019-10-30T08:59:03Z

Resolves #973. See the proposal in #973 (comment).

Changelog-friendly one-liner: Unpin commented out unsafe packages in requirements.txt.

Contributor checklist

Provided the tests for the changes.
Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

PeterJCLaw

👍 thanks for putting this together so quickly.

Code changes look good (though I've not looked at this codebase before), and it appears to work as advertised in my codebase.

codecov · 2019-10-30T10:18:22Z

Codecov Report

Merging #975 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #975      +/-   ##
=========================================
+ Coverage    99.1%   99.1%   +<.01%     
=========================================
  Files          34      34              
  Lines        2357    2358       +1     
  Branches      305     305              
=========================================
+ Hits         2336    2337       +1     
  Misses         11      11              
  Partials       10      10

Impacted Files	Coverage Δ
tests/test_writer.py	`100% <ø> (ø)`	⬆️
tests/test_cli_compile.py	`100% <ø> (ø)`	⬆️
piptools/writer.py	`99.2% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5efb2de...841bffb. Read the comment docs.

atugushev · 2019-11-14T18:29:11Z

It turns out it was already discussed before, see #276 (comment), and fixed in #294, but has been broken later unintentionally.

webknjaz · 2019-11-15T18:16:45Z

tests/test_cli_compile.py

@@ -691,8 +691,7 @@ def test_annotate_option(pip_conf, runner, option, expected):


 @pytest.mark.parametrize(
-    "option, expected",
-    [("--allow-unsafe", "\nsetuptools=="), (None, "\n# setuptools==")],
+    "option, expected", [("--allow-unsafe", "\nsetuptools=="), (None, "\n# setuptools")]


I don't see any difference in this change. Looks unrelated...

Oh, I see. The second one doesn't have ==. It's been hard to spot. I suggest that you minimize the diff in order to make it review-friendly.

Yeah, that's why I've split the PR into two commits.

Well, it's not visible in the PR diff and folks mostly don't do reviews per commit (I do sometimes but to all the time).

OTOH, you mix up two separate tasks in this PR:

reformatting the code

changing things

This makes it non-atomic, which is not good in my book. As you can see, this confuses even me. And I do a lot more reviews than most of the regular folks (compared to other types of contributions). Which opens up possibilities to sneak in a few more bugs unnoticed. Humans are error-prone and byte-by-byte comparisons are hard without any highlighting.

Yeah, it should have been separate PRs. I thought these changes would be easy to review (which is apparently not). My bad. Would you like me to split this into two PRs or we could go on?

OTOH if you look to this diff line https://github.com/jazzband/pip-tools/pull/975/files#diff-9960f0e0139a2d6000305af0c80dac37R194 you'll see a part of the string with highlighted which is what our goal should be.

That's what I'm talking about!

Not that I know of. But whenever I start it, everybody starts hating me for this. So I'm not very enthusiastic about participating in yet another argument personally...

Haha! That's a rough path 😄

Anyway. I think that one commit should have a logic change and then autoformatting should be a separate commit.
Or just merge it if you want.

That makes sense, but as you mentioned before:

Well, it's not visible in the PR diff and folks mostly don't do reviews per commit (I do sometimes but to all the time).

And I can't merge this unless someone approves it.

Oh, that approval from @PeterJCLaw doesn't work..

as you mentioned before

I'd like to point out that both individual commits and PRs can be atomic, they just refer to different abstraction levels.

piptools/writer.py

webknjaz · 2019-11-15T18:32:57Z

piptools/writer.py

                if not self.allow_unsafe:
-                    yield comment("# {}".format(req))
+                    yield comment("# {}".format(ireq_key))


If you stick continue here, you could dedent the following code block

Suggested change

yield comment("# {}".format(ireq_key))

yield comment("# {}".format(ireq_key))

continue

You probably right, but I think it's not related to the issue. I'd prefer to touch these lines in a separate PR. First things first.

And it looks subjective, is there a PEP (or guidelines) which suggests to use:

for x in z: if x: foo() continue bar()

instead of

for x in z: if x: foo() else: bar()

? Just wondering.

Yes, it's subjective. I don't recall this being mentioned in any PEPs.

It reduces the indentation which is better for readability

There's a similar pylint rule for return
def func(): if cond1: return xxx else: # pylint would emit a warning here return yyy
I think it's reasonable to apply this rule to other similar structures.

Yes, it makes sense to do this in a separate refactoring PR to keep the diff small.

tests/test_cli_compile.py

atugushev · 2019-11-17T17:57:32Z

@webknjaz thanks for reviewing this!

atugushev mentioned this pull request Oct 30, 2019

pip-compile bumps setuptools within commented-out spec #973

Closed

atugushev added the enhancement Improvements to functionality label Oct 30, 2019

PeterJCLaw approved these changes Oct 30, 2019

View reviewed changes

atugushev added this to the 4.3.0 milestone Oct 30, 2019

atugushev requested review from codingjoe and webknjaz November 14, 2019 18:27

webknjaz reviewed Nov 15, 2019

View reviewed changes

piptools/writer.py Show resolved Hide resolved

webknjaz reviewed Nov 15, 2019

View reviewed changes

piptools/writer.py Show resolved Hide resolved

webknjaz reviewed Nov 15, 2019

View reviewed changes

tests/test_cli_compile.py Show resolved Hide resolved

atugushev mentioned this pull request Nov 15, 2019

Refactor-out unassertive OutputWriter() tests #986

Merged

Unpin commented out unsafe package in requirements.txt

841bffb

atugushev force-pushed the unpin-commented-out-unsafe-pkgs branch from 9c82c89 to 841bffb Compare November 16, 2019 07:14

atugushev commented Nov 16, 2019

View reviewed changes

tests/test_cli_compile.py Show resolved Hide resolved

webknjaz mentioned this pull request Nov 16, 2019

Pin setuptools in test_allow_unsafe_option #988

Closed

webknjaz approved these changes Nov 16, 2019

View reviewed changes

atugushev merged commit ef52977 into jazzband:master Nov 17, 2019

atugushev deleted the unpin-commented-out-unsafe-pkgs branch November 17, 2019 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unpin commented out unsafe packages #975

Unpin commented out unsafe packages #975

atugushev commented Oct 30, 2019 •

edited

Loading

PeterJCLaw left a comment

codecov bot commented Oct 30, 2019 •

edited

Loading

atugushev commented Nov 14, 2019

webknjaz Nov 15, 2019

webknjaz Nov 15, 2019

atugushev Nov 15, 2019

webknjaz Nov 15, 2019

atugushev Nov 15, 2019

atugushev Nov 16, 2019

webknjaz Nov 16, 2019

atugushev Nov 16, 2019

webknjaz Nov 16, 2019

webknjaz Nov 16, 2019

webknjaz Nov 15, 2019

atugushev Nov 15, 2019

atugushev Nov 15, 2019 •

edited

Loading

webknjaz Nov 15, 2019

atugushev commented Nov 17, 2019

	yield comment("# {}".format(ireq_key))
	yield comment("# {}".format(ireq_key))
	continue

Unpin commented out unsafe packages #975

Unpin commented out unsafe packages #975

Conversation

atugushev commented Oct 30, 2019 • edited Loading

Contributor checklist

PeterJCLaw left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 30, 2019 • edited Loading

Codecov Report

atugushev commented Nov 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atugushev Nov 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atugushev commented Nov 17, 2019

atugushev commented Oct 30, 2019 •

edited

Loading

codecov bot commented Oct 30, 2019 •

edited

Loading

atugushev Nov 15, 2019 •

edited

Loading