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.

Sign up for GitHub

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

Jump to bottom

Unpin commented out unsafe packages #975

Merged
merged 1 commit into from
Nov 17, 2019
Merged

Unpin commented out unsafe packages #975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions piptools/writer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,17 +189,18 @@ def _iter_lines(
yield MESSAGE_UNSAFE_PACKAGES

for ireq in unsafe_requirements:
req = self._format_requirement(
ireq,
reverse_dependencies,
primary_packages,
marker=markers.get(key_from_ireq(ireq)),
hashes=hashes,
)
ireq_key = key_from_ireq(ireq)
webknjaz marked this conversation as resolved.
Show resolved Hide resolved
if not self.allow_unsafe:
yield comment("# {}".format(req))
yield comment("# {}".format(ireq_key))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@atugushev atugushev Nov 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

  1. It reduces the indentation which is better for readability
  2. 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.
  3. Yes, it makes sense to do this in a separate refactoring PR to keep the diff small.

else:
yield req
line = self._format_requirement(
webknjaz marked this conversation as resolved.
Show resolved Hide resolved
ireq,
reverse_dependencies,
primary_packages,
marker=markers.get(ireq_key),
hashes=hashes,
)
yield line

# Yield even when there's no real content, so that blank files are written
if not yielded:
Expand Down
3 changes: 1 addition & 2 deletions tests/test_cli_compile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. reformatting the code
  2. 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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 😄

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

webknjaz marked this conversation as resolved.
Show resolved Hide resolved
webknjaz marked this conversation as resolved.
Show resolved Hide resolved
)
@pytest.mark.network
def test_allow_unsafe_option(runner, option, expected):
Expand Down
4 changes: 2 additions & 2 deletions tests/test_writer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ def test_iter_lines__unsafe_dependencies(writer, from_line, allow_unsafe):
"test==1.2",
"",
MESSAGE_UNSAFE_PACKAGES,
"setuptools==1.10.0" if allow_unsafe else comment("# setuptools==1.10.0"),
"setuptools==1.10.0" if allow_unsafe else comment("# setuptools"),
)
assert tuple(lines) == expected_lines

Expand All @@ -147,7 +147,7 @@ def test_iter_lines__unsafe_with_hashes(writer, from_line):
"test==1.2 \\\n --hash=FAKEHASH",
"",
MESSAGE_UNSAFE_PACKAGES_UNPINNED,
comment("# setuptools==1.10.0"),
comment("# setuptools"),
)
assert tuple(lines) == expected_lines

Expand Down