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.

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

Ensure pip-compile --no-header <blank requirements.in> creates/overwrites requirements.txt #909

Merged
merged 1 commit into from
Oct 1, 2019

Conversation

AndydeCleyre
Copy link
Contributor

@AndydeCleyre AndydeCleyre commented Sep 24, 2019

Before, in the case where the writer should generate a blank output document (e.g. requirements.txt), no new file would be created, and any existing file with that name would be left unchanged. With this change, a blank document will created and overwritten if it exists.

This is implemented by having the writer keep track of whether it yields any lines, and if not, yielding an empty string (where before it would yield nothing).

A simpler solution would be to always yield an extra empty string at the end of _iter_lines, but that would result in a bit of awkward space at the bottom of all the txts, and if that were fixed it would involve leaking the composition logic outside of _iter_lines.

Changelog-friendly one-liner: pip-compile --no-header <blank requirements.in> now creates/overwrites requirements.txt.

Fixes #900.

@atugushev I hereby request a review. If this does not satisfy the "Requested a review from another contributor" requirement, please tell me what does. Thanks!

Contributor checklist
  • Provided the tests for the changes.
  • Requested a review from another contributor.
  • 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).

Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

Thank you for submitting this! I was thinking about:

@@ -230,6 +220,10 @@ class OutputWriter(object):
             if not self.dry_run:
                 self.dst_file.write(unstyle(line).encode("utf-8"))
                 self.dst_file.write(os.linesep.encode("utf-8"))
+        else:
+            # Create an empty file, if there is no output
+            if not self.dry_run:
+                self.dst_file.write("")

What do you think?

@atugushev
Copy link
Member

@AndydeCleyre could you please rebase onto master to fix red CI checks?

@atugushev atugushev changed the title Bugfix/900 - Ensure pip-compile --no-header <blank requirements.in> creates/overwrites requirements.txt Ensure pip-compile --no-header <blank requirements.in> creates/overwrites requirements.txt Sep 26, 2019
@codecov
Copy link

codecov bot commented Sep 26, 2019

Codecov Report

Merging #909 into master will decrease coverage by 0.47%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #909      +/-   ##
==========================================
- Coverage   99.06%   98.58%   -0.48%     
==========================================
  Files          35       35              
  Lines        2247     2264      +17     
  Branches      288      290       +2     
==========================================
+ Hits         2226     2232       +6     
- Misses         13       21       +8     
- Partials        8       11       +3
Impacted Files Coverage Δ
tests/test_cli_compile.py 100% <100%> (ø) ⬆️
piptools/writer.py 99.2% <100%> (+0.05%) ⬆️
piptools/repositories/pypi.py 91.21% <0%> (-3.91%) ⬇️
piptools/utils.py 97.95% <0%> (-2.05%) ⬇️

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 25a4e99...d53d3ff. Read the comment docs.

@AndydeCleyre
Copy link
Contributor Author

Is a simple merge of master into here acceptable? GitHub pull requests don't love history rewrites.

I may be misunderstanding your alternative suggested fix, but:

  1. If that's an else component of the for loop, that empty string would be written whenever the for loop finishes without interruption from a break statement, which would be every time, as there are no break statements. So the simpler version of this solution would be to add the extra dry run check and blank-write before the for loop.

  2. While that will work, I thought changing the behavior of _iter_lines would be a less fragile solution, as it doesn't create a difference between what _iter_lines yields and what write produces, noting that _iter_lines is used directly by three existing tests.

That said, always writeing an empty string is simpler, and I'm happy to create a replacement pull request with that, cherry picking the commit from here which adds the test, and double-checking that other tests are not broken.

Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

My suggestion was wrong, thanks for noticing. I'm okay with the current approach, but I have a few comments.

piptools/writer.py Outdated Show resolved Hide resolved
piptools/writer.py Outdated Show resolved Hide resolved
piptools/writer.py Outdated Show resolved Hide resolved
piptools/writer.py Outdated Show resolved Hide resolved
Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@atugushev atugushev added this to the 4.2.0 milestone Oct 1, 2019
@atugushev
Copy link
Member

Could you rebase and squash before I merge this pull request?

…d --no-header is supplied (Fixes jazzband#900)

Add !is_empty (yielded) tracking to the writer's _iter_lines method, and yield a blank line (empty string) if (and only if) otherwise nothing at all would be yielded.
@atugushev atugushev merged commit fd8aee7 into jazzband:master Oct 1, 2019
@atugushev
Copy link
Member

Thanks, @AndydeCleyre!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pip-compile --no-header <blank requirements.in> neither creates nor overwrites requirements.txt
2 participants