Skip to content

Conversation

@caphrim007
Copy link
Contributor

This patch changes hashin such that,

If the package specified has underscores in its name, but the new package
has hyphens, the following occurs

  • If there is a new version, hashin updates the package in requirements to
    be the hyphenated name
  • If there is no new version, the underscored name remains in place

Prior to this patch, the 1st case about would not occur. Instead, hashin
would append the new version in its hyphenated form to the requirements file
and leave the old version in its underscored form in the requirements file.

This patch changes hashin such that,

If the package specified has underscores in its name, but the new package
has hyphens, the following occurs

* If there is a new version, hashin updates the package in requirements to
  be the hyphenated name
* If there is no new version, the underscored name remains in place

Prior to this patch, the 1st case about would not occur. Instead, hashin
would append the new version in its hyphenated form to the requirements file
and leave the old version in its underscored form in the requirements file.
@caphrim007
Copy link
Contributor Author

@peterbe ready for review

@caphrim007 caphrim007 changed the title Fixes #116 Correct writing of hyphenated and underscored package names Jun 11, 2020
@caphrim007
Copy link
Contributor Author

Fixes #116

Copy link
Owner

@peterbe peterbe left a comment

Choose a reason for hiding this comment

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

I did some pretty basic (manual) testing it all seems to work out:

▶ cat /tmp/reqs.txt
readme_renderer==25.0 \
    --hash=sha256:cbe9db71defedd2428a1589cdc545f9bd98e59297449f69d721ef8f1cfced68d \
    --hash=sha256:cc4957a803106e820d05d14f71033092537a22daa4f406dfbdd61177e0936376
python_memcached==1.58 \
    --hash=sha256:4dac64916871bd3550263323fc2ce18e1e439080a2d5670c594cf3118d99b594 \
    --hash=sha256:a2e28637be13ee0bf1a8b6843e7490f9456fd3f2a4cb60471733c7b5d5557e4f

▶ python hashin.py -u -i /tmp/reqs.txt
PACKAGE                        YOUR VERSION    NEW VERSION
readme-renderer                25.0            26.0            ✓
python-memcached               1.58            1.59            ✓

▶ cat /tmp/reqs.txt
readme-renderer==26.0 \
    --hash=sha256:cbe9db71defedd2428a1589cdc545f9bd98e59297449f69d721ef8f1cfced68d \
    --hash=sha256:cc4957a803106e820d05d14f71033092537a22daa4f406dfbdd61177e0936376
python-memcached==1.59 \
    --hash=sha256:4dac64916871bd3550263323fc2ce18e1e439080a2d5670c594cf3118d99b594 \
    --hash=sha256:a2e28637be13ee0bf1a8b6843e7490f9456fd3f2a4cb60471733c7b5d5557e4f

▶ python hashin.py -u -i /tmp/reqs.txt

▶ echo $?
0

And this one:

▶ python hashin.py -r /tmp/reqs.txt python_memcached==1.58

▶ cat /tmp/reqs.txt
readme-renderer==26.0 \
    --hash=sha256:cbe9db71defedd2428a1589cdc545f9bd98e59297449f69d721ef8f1cfced68d \
    --hash=sha256:cc4957a803106e820d05d14f71033092537a22daa4f406dfbdd61177e0936376
python-memcached==1.58 \
    --hash=sha256:2775829cb54b9e4c5b3bbd8028680f0c0ab695db154b9c46f0f074ff97540eb6

Looks great.
I can't think of any other ways to test it.
I like that if you already had some_package==X.Y.Z where X.Y.Z is the latest version, running hashin -u -i just plainly ignores it. I.e. it doesn't try to correct the name if the version doesn't need to change.

The only thing is the nit.

hashin.py Outdated
# in the lines being compared. This results in "old" names matching
# "new" names so that hashin correctly replaces them when it looks for
# them.
rex = re.compile("[-_]")
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't that be...

Suggested change
rex = re.compile("[-_]")
rex = re.compile(r"[-_]")

A bit surprised that flake8 didn't complain about that.

Also, rex isn't a great name. It sounds like a pun on "regular expression" and it's not mentioning anything about its purpose in life or its use.

3.4: py34
3.5: py35
3.6: py36, lint, restlint
3.7: py37, lint, restlint
Copy link
Owner

Choose a reason for hiding this comment

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

Note-to-self; we should probably drop 3.4 and add 3.8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want me to change this in this PR? lemme know

Copy link
Owner

Choose a reason for hiding this comment

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

I have a new PR coming that'll also do this. So leave it as is.

hashin.py Outdated
# different.
# The 'new_lines` is what we might intend to replace it with.
old = set([l.strip(" \\") for l in old_lines])
old = set([rex.sub("-", l.strip(" \\")) for l in old_lines])
Copy link
Owner

Choose a reason for hiding this comment

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

This will fail newer versions of flake8 because l is a bad variable name. It's not your fault but do you mind
changing this to:

Suggested change
old = set([rex.sub("-", l.strip(" \\")) for l in old_lines])
old = set([rex.sub("-", line.strip(" \\")) for line in old_lines])

whilst you're at it.

@caphrim007
Copy link
Contributor Author

@peterbe I will send updates shortly. thanks for reviewing!

@caphrim007 caphrim007 requested a review from peterbe June 15, 2020 17:13
* Drop python 3.4 from tox.ini
* Add Python 3.8 to tox.ini
* Correct variable names
* Change string to regex string
@caphrim007 caphrim007 force-pushed the feature.hyphens-and-underscores branch from fad0d58 to e7a61dd Compare June 15, 2020 17:16
@caphrim007
Copy link
Contributor Author

changes made. Ready for review

@peterbe
Copy link
Owner

peterbe commented Jun 15, 2020

Travis isn't reporting checks here on the PR, but: https://travis-ci.org/github/peterbe/hashin/builds/698623654 is all green!

@peterbe peterbe merged commit bcc3b2d into peterbe:master Jun 15, 2020
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.

2 participants