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

Replace packaging with packvers #108 #109

Merged
merged 1 commit into from
Jan 13, 2023

Conversation

swastkk
Copy link
Contributor

@swastkk swastkk commented Jan 3, 2023

Packvers added in resolution.py and changed the version of pip-requirments-parser too.
for the aboutcode-org/scancode.io#576 and #108 issue as well.
Signed-off-by: Swastik Sharma [email protected]

@swastkk swastkk force-pushed the packvers-added branch 2 times, most recently from a04b5f4 to e7baa79 Compare January 3, 2023 19:39
@swastkk
Copy link
Contributor Author

swastkk commented Jan 3, 2023

After analysing I found that due to that change in Packvers, we need to update our https://github.com/nexB/dparse2 for packvers and packaging change in the release and https://github.com/nexB/pip-requirements-parser to in the release , after that this PR will have all Test pass
Signed-off-by: swastik [email protected]

Copy link
Member

@pombredanne pombredanne 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 ++ !

"Packvers added" may not be the best name for this PR. What about "Replace packaging with packvers" instead?

Also on the commit message style, your use of things such as "packaging-to-packvers-done-in-resolution.py" may not be something we typically use... look for other commit messages for inspiration and also at https://aboutcode.readthedocs.io/en/latest/contributing/writing_good_commit_messages.html

See also the other review comments I posted for your kind consideration.

etc/scripts/utils_dejacode.py Outdated Show resolved Hide resolved
src/_packagedcode/pypi.py Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@swastkk
Copy link
Contributor Author

swastkk commented Jan 5, 2023

Thank you ++ !

"Packvers added" may not be the best name for this PR. What about "Replace packaging with packvers" instead?

Also on the commit message style, your use of things such as "packaging-to-packvers-done-in-resolution.py" may not be something we typically use... look for other commit messages for inspiration and also at https://aboutcode.readthedocs.io/en/latest/contributing/writing_good_commit_messages.html

See also the other review comments I posted for your kind consideration.

Okay sure will change accordingly : )

@swastkk swastkk changed the title Packvers added Replace packaging with packvers Jan 5, 2023
Copy link
Member

@pombredanne pombredanne 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 ... here are some review comments

etc/scripts/utils_pip_compatibility_tags.py Outdated Show resolved Hide resolved
etc/scripts/utils_thirdparty.py Outdated Show resolved Hide resolved
src/python_inspector/utils_pypi.py Outdated Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
src/python_inspector/api.py Outdated Show resolved Hide resolved
Copy link
Member

@pombredanne pombredanne 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 the updates. Do you mind to squash your commits in a single commit with a good commit message?

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

LGTM and ready to merge once squashed!

@swastkk
Copy link
Contributor Author

swastkk commented Jan 11, 2023

LGTM and ready to merge once squashed!

Done ✔️, can merge now :")

@pombredanne
Copy link
Member

@swastkk you did not squash it all in one commit, you still have 15 commits... you squash and force push. https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History

@swastkk
Copy link
Contributor Author

swastkk commented Jan 12, 2023

@swastkk you did not squash it all in one commit, you still have 15 commits... you squash and force push. https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History

Is it okay now??

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

Thanks!
Can you make one last change and amend your commit?

We want to subject to be using imperative style (e.g., Replace not Replaced) and to be subject line must be under 50 characters and ew extra things to review at https://aboutcode.readthedocs.io/en/latest/contributing/writing_good_commit_messages.html

@swastkk swastkk changed the title Replace packaging with packvers Replace packaging with packvers to issue https://github.com/nexB/python-inspector/issues/108 Jan 12, 2023
@swastkk swastkk changed the title Replace packaging with packvers to issue https://github.com/nexB/python-inspector/issues/108 Replace packaging with packvers to issue #108 Jan 12, 2023
@swastkk swastkk changed the title Replace packaging with packvers to issue #108 Replace packaging with packvers (issue #108) Jan 12, 2023
@swastkk
Copy link
Contributor Author

swastkk commented Jan 12, 2023

Thanks! Can you make one last change and amend your commit?

We want to subject to be using imperative style (e.g., Replace not Replaced) and to be subject line must be under 50 characters and ew extra things to review at https://aboutcode.readthedocs.io/en/latest/contributing/writing_good_commit_messages.html

Now?

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

Thanks!
"Replace packaging with packvers and bump packages version" is still at 57 chars... can you do something more or less like this instead:

Replace packaging library with packvers #108

Work around changes in packaging by replacing it with packvers
to avoid issues with missing packaging.version.LegacyVersion.

Also bump the versions of dparse2 and pip-requirements-parser
with new versions that are not subject the LegcacyVersion issue.

Adjust the code and tests accordingly.

Reference: https://github.com/nexB/python-inspector/issues/108
Reference: https://github.com/pypa/packaging/issues/530
Signed-off-by: swastik <[email protected]>

And please remove all changes you made that are not strictly about this... I am not sure what happened but you end up with unrelated changes. Do not hesitate to ask for help on the Gitter chat!

Thanks again for your preseverance.

CHANGELOG.rst Outdated Show resolved Hide resolved
src/python_inspector/api.py Outdated Show resolved Hide resolved
src/python_inspector/api.py Outdated Show resolved Hide resolved
src/python_inspector/resolve_cli.py Outdated Show resolved Hide resolved
src/python_inspector/resolve_cli.py Outdated Show resolved Hide resolved
src/python_inspector/resolve_cli.py Outdated Show resolved Hide resolved
@swastkk swastkk force-pushed the packvers-added branch 3 times, most recently from 53934bf to 2a36d33 Compare January 12, 2023 16:32
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

Thanks for the update. One last nit before we can merge.

src/_packagedcode/pypi.py Show resolved Hide resolved
@pombredanne pombredanne changed the title Replace packaging with packvers (issue #108) Replace packaging with packvers #108 Jan 12, 2023
@swastkk swastkk force-pushed the packvers-added branch 2 times, most recently from ea761e5 to 0bdaa36 Compare January 13, 2023 08:58
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

Here are a few last nits.

src/_packagedcode/pypi.py Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
* Work around changes in packaging by replacing it with packvers
to avoid issues with missing packaging.version.LegacyVersion.

* Also bump the versions of dparse2 and pip-requirements-parser
with new versions that are not subject the LegcacyVersion issue.

* Adjust the code and tests accordingly.

Reference: aboutcode-org#108
Reference: pypa/packaging#530
Signed-off-by: swastik <[email protected]>
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

All good .... Thank you ++

@pombredanne pombredanne merged commit e1ffca1 into aboutcode-org:main Jan 13, 2023
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.

3 participants