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

Normalize «command to run» in pip-compile headers. #800

Merged
merged 3 commits into from
May 5, 2019

Conversation

atugushev
Copy link
Member

@atugushev atugushev commented Apr 27, 2019

Normalizes To update, run: header in pip-compile. To normalize it:

  • expands short options to long
  • removes values that are already default
  • sorts the arguments alphabetically
  • sorts variadic arguments (src files) alphabetically and puts them to the tail of the command
  • removes arguments that don't change build behaviour like --verbose or --quiet
  • removes one-off options like --rebuild or --upgrade

Closes #788, #799.

Also fixed an error when requirements.txt has unicode chars.

Changelog-friendly one-liner: Normalize «command to run» in pip-compile headers.

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).

@atugushev atugushev added the enhancement Improvements to functionality label Apr 27, 2019
@atugushev atugushev force-pushed the normalize-compile-command-header branch from 19a8b6d to c697c5d Compare April 27, 2019 14:05
@codecov
Copy link

codecov bot commented Apr 27, 2019

Codecov Report

Merging #800 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #800      +/-   ##
==========================================
+ Coverage   98.78%   98.82%   +0.03%     
==========================================
  Files          35       36       +1     
  Lines        2050     2119      +69     
  Branches      262      275      +13     
==========================================
+ Hits         2025     2094      +69     
  Misses         15       15              
  Partials       10       10
Impacted Files Coverage Δ
piptools/scripts/compile.py 100% <ø> (ø) ⬆️
piptools/utils.py 100% <100%> (ø) ⬆️
tests/test_utils.py 100% <100%> (ø) ⬆️
piptools/writer.py 100% <100%> (ø) ⬆️
piptools/__init__.py 100% <100%> (ø)
tests/test_writer.py 100% <100%> (ø) ⬆️
tests/conftest.py 95.12% <100%> (-0.06%) ⬇️

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 c45998a...5cb1764. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 27, 2019

Codecov Report

Merging #800 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #800      +/-   ##
==========================================
- Coverage   98.06%   98.01%   -0.05%     
==========================================
  Files          34       34              
  Lines        1965     1969       +4     
  Branches      257      269      +12     
==========================================
+ Hits         1927     1930       +3     
  Misses         26       26              
- Partials       12       13       +1
Impacted Files Coverage Δ
piptools/scripts/compile.py 100% <ø> (ø) ⬆️
piptools/utils.py 100% <100%> (ø) ⬆️
tests/test_utils.py 100% <100%> (ø) ⬆️
tests/test_writer.py 100% <100%> (ø) ⬆️
piptools/writer.py 100% <100%> (ø) ⬆️
tests/test_sync.py 98.64% <0%> (-1.36%) ⬇️
piptools/repositories/local.py 90.69% <0%> (-0.8%) ⬇️
tests/conftest.py 94.52% <0%> (-0.67%) ⬇️
piptools/cache.py 94.52% <0%> (-0.08%) ⬇️
piptools/scripts/sync.py 100% <0%> (ø) ⬆️
... and 14 more

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 5da4a25...c697c5d. Read the comment docs.

Copy link
Contributor

@blueyed blueyed left a comment

Choose a reason for hiding this comment

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

Very nice attentions to details! :)
(only reviewed it quickly.)

piptools/utils.py Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
tests/test_writer.py Outdated Show resolved Hide resolved
@adamchainz
Copy link
Contributor

Did a quick test, works for me. Also the trailing space is gone from the default argument-less pip-compile, yay, that always got stripped by my editor causing diff noise.

$ pip install -e 'git+https://github.com/atugushev/pip-tools.git@c697c5d935bd93bd29ca48ac2749a21b96b01526#egg=piptools'
Obtaining piptools from git+https://github.com/atugushev/pip-tools.git@c697c5d935bd93bd29ca48ac2749a21b96b01526#egg=piptools
  ...
Successfully installed click-7.0 pip-tools six-1.12.0
$ echo django > requirements.in
$ python -m piptools compile
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile
#
django==2.2
pytz==2019.1              # via django
sqlparse==0.3.0           # via django
$ st requirements.txt
$ python -m piptools compile --upgrade-package django
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile
#
django==2.2
pytz==2019.1              # via django
sqlparse==0.3.0           # via django

piptools/utils.py Outdated Show resolved Hide resolved
piptools/utils.py Outdated Show resolved Hide resolved
@@ -9,6 +9,14 @@
from .click import style

UNSAFE_PACKAGES = {"setuptools", "distribute", "pip"}
COMPILE_EXCLUDE_OPTIONS = {
Copy link
Member

Choose a reason for hiding this comment

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

any way to extend the click option decorator with a flag for compile_exclude?

Copy link
Member Author

Choose a reason for hiding this comment

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

May be there is a way. Need to research.

Copy link
Contributor

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

Please rebase as well when you restore the effort.

@atugushev atugushev changed the title Normalize «command to run» in pip-compile headers. WIP: Normalize «command to run» in pip-compile headers. May 4, 2019
@atugushev atugushev force-pushed the normalize-compile-command-header branch 2 times, most recently from 1c7c617 to 7b4fc90 Compare May 4, 2019 18:26
@atugushev atugushev force-pushed the normalize-compile-command-header branch 2 times, most recently from f2c3ada to d626752 Compare May 5, 2019 05:01
Fixes raising an exception on pip._internal.utils.encoding.auto_decode
when requirement.txt has unique symblols.
@atugushev atugushev force-pushed the normalize-compile-command-header branch from d626752 to 5cb1764 Compare May 5, 2019 05:05
@atugushev atugushev requested a review from blueyed May 5, 2019 16:22
@atugushev atugushev changed the title WIP: Normalize «command to run» in pip-compile headers. Normalize «command to run» in pip-compile headers. May 5, 2019
@atugushev
Copy link
Member Author

atugushev commented May 5, 2019

I've added review fixes. Please review guys.

@auvipy auvipy merged commit 990a3d0 into jazzband:master May 5, 2019
@atugushev
Copy link
Member Author

@auvipy please don't hurry up to merge PRs, give members a chance to review.

@atugushev atugushev added this to the 3.7.0 milestone May 6, 2019
@atugushev
Copy link
Member Author

@blueyed @graingert if you have any comments you can post here and i'll try to resolve them in a different PR.

@auvipy
Copy link
Contributor

auvipy commented May 10, 2019

It seems I might prematurely merge this. Issue reported of regression #811 could you verify?

@atugushev
Copy link
Member Author

@auvipy it's confirmed. See #811 (comment). The case with env variables has been forgotten.

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

Successfully merging this pull request may close these issues.

Nomalize 'To update, run' advice
6 participants