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

Draft licenses CLI feature #1433

Merged
merged 45 commits into from
Oct 28, 2022
Merged

Draft licenses CLI feature #1433

merged 45 commits into from
Oct 28, 2022

Conversation

JohnHardy
Copy link
Contributor

@JohnHardy JohnHardy commented Oct 13, 2022

Pull Request Check List

  • A news fragment is added in news/ describing what is new.
  • Test cases added for changed code.

Describe what you have changed in this PR.

Added a new CLI command licenses that allows you to output the license documentation for all the deps. This usually requires 3rd party projects that may or may not be compatible with whatever packaging system is used. This CLI command implements common use cases that I have seen in the wild.

🙏🙏 If you are interested in merging this PR in, please let me know so I will invest the time to lint / write test cases. However, before that, I wanted to make sure that you were happy with: (1) the existence of the feature, (2) the implementation details, my first time working on a package manager, (3) the ux. 🙏🙏

Features are:

  • Licenses can be gathered from the working set or resolved using the resolver/lockfile.
  • Output to CSV
  • Output to JSON
  • Output to console in the fancy PDM tables
  • Output as markdown (including license text files included in dist-info)
  • If a License field is empty, it will try to recover it from the Trove classifiers.
  • Highlight the group a requirement is defined in (eg. dev deps)
  • Distinguish between specified deps and sub-dependencies

Here are some use cases:

pdm licenses
image

pdm licenses --resolve --skip default,tox,workflow,sub --fields group,identifier,version,licenses --sort licenses
image

pdm licenses --sort licenses
image

pdm licenses --sort licenses --csv
image

pdm licenses --sort licenses --markdown > mylicenses.md
image

@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2022

Codecov Report

Base: 84.97% // Head: 85.11% // Increases project coverage by +0.14% 🎉

Coverage data is based on head (b1815a1) compared to base (1e5dc62).
Patch coverage: 89.79% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1433      +/-   ##
==========================================
+ Coverage   84.97%   85.11%   +0.14%     
==========================================
  Files          88       88              
  Lines        7662     7836     +174     
  Branches     1798     1852      +54     
==========================================
+ Hits         6511     6670     +159     
- Misses        766      779      +13     
- Partials      385      387       +2     
Flag Coverage Δ
unittests 84.92% <89.79%> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pdm/cli/actions.py 79.46% <ø> (-1.05%) ⬇️
src/pdm/cli/commands/list.py 90.19% <89.74%> (-9.81%) ⬇️
src/pdm/cli/utils.py 81.26% <100.00%> (+2.11%) ⬆️
src/pdm/models/repositories.py 78.04% <0.00%> (-0.82%) ⬇️
src/pdm/termui.py 89.85% <0.00%> (-0.73%) ⬇️
src/pdm/models/requirements.py 95.10% <0.00%> (+0.30%) ⬆️
src/pdm/resolver/providers.py 94.57% <0.00%> (+0.77%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@frostming
Copy link
Collaborator

frostming commented Oct 13, 2022

The output is very similar to pdm list and it is preferred to extend that command rather than adding a new one. Or is it better to live in its own plugin?

@JohnHardy
Copy link
Contributor Author

JohnHardy commented Oct 13, 2022

@frostming sounds good to me, but it will impact other functions inside pdm like show_dependency_graph , actions.py, etc. I was reluctant to pollute do_list since I don't know the codebase. What do you prefer?

Shall I keep the switches for selecting fields, skipping dep groups, changing the sort column, etc?

EDIT: It could also live in a plugin - happy to follow your guidance and suggestions from others.

@frostming
Copy link
Collaborator

frostming commented Oct 14, 2022

@frostming sounds good to me, but it will impact other functions inside pdm like show_dependency_graph , actions.py, etc. I was reluctant to pollute do_list since I don't know the codebase. What do you prefer?

Shall I keep the switches for selecting fields, skipping dep groups, changing the sort column, etc?

EDIT: It could also live in a plugin - happy to follow your guidance and suggestions from others.

The column selection is great, let's revisit the current options and the options to be added, if it is to be integrated in to pdm list:

Format/Option --fields --sort --resolve --license --reverse --include/--exclude1
table
csv
json
markdown
freeze 2
graph3

I hope that doesn't make the feature too complex.

Footnotes

  1. To replace the proposed --skip, they should only be allowed with --resolve, and include is prior to exclude

  2. should use pdm export --format requirements instead

  3. All format options are exclusive except --graph can be given with --json

@frostming frostming added RFC ⭐ enhancement Improvements for existing features labels Oct 14, 2022
@JohnHardy
Copy link
Contributor Author

I had another quick go at this again to match your feature matrix, let me know what you think!

Let me know if I missed anything or you would like something to be done differently.

Questions:

  1. Currently using --include * to mean include everything. Do you want it to match pdm install -G:all style instead?
  2. Currently using --exclude sub to mean exclude sub-dependencies, but sub seems like an unclear choice of word. Any suggestions?
  3. At the moment there is the case where cachecontrol and cachecontrol[filecache] reference the cachecontrol package (seems right). Is there a case where they should be distinguished by name? Is there a way of mapping distributions back to requirement identifiers.
  4. At the moment, the resolver works on ALL groups BEFORE being filtered and listed. However, we might want to send less packages to the resolver. Only I thought that could potentially resolve different versions / licenses. Probably nobody will care, but just wondering.

If you are happy, I can make some test cases.

PDM list

@frostming
Copy link
Collaborator

  1. Currently using --include * to mean include everything. Do you want it to match pdm install -G:all style instead?

The current implementation LGTM.

  1. Currently using --exclude sub to mean exclude sub-dependencies, but sub seems like an unclear choice of word. Any suggestions?

Two options, 1) use a special identifier to avoid conflicting with existing groups, such as :sub 2) use another option flag --exclude-sub

  1. At the moment there is the case where cachecontrol and cachecontrol[filecache] reference the cachecontrol package (seems right).

IMO it is okay to ignore the entries with extras []

@JohnHardy
Copy link
Contributor Author

JohnHardy commented Oct 15, 2022

  1. 👍
  2. Done, :sub is used. 👍
  3. Sounds good. 👍 FYI: I noticed that the echo function will trim word[here] to word even if the string does not contain [/] to close it.
  4. I am updating the tests now. Please can you advise how to mock the Command functions?

For example:

def test_list_command(project, invoke, mocker):  # previous test
    do_list = mocker.patch.object(actions, "do_list")
    invoke(["list"], obj=project)
    do_list.assert_called_once()   # <--- works OK in old codebase
    
def test_list_command(project, invoke, mocker):  # proposed new test
    handle = mocker.patch.object(Command, "handle")
    invoke(["list"], obj=project)
    handle.assert_called_once()   # <--- list.Command.handle is not called

@frostming
Copy link
Collaborator

frostming commented Oct 17, 2022

3. FYI: I noticed that the echo function will trim word[here] to word even if the string does not contain [/] to close it.

Yeah, that is the markup syntax of rich console, [] need to be escaped by \

4. I am updating the tests now. Please can you advise how to mock the Command functions?

These tests around do_list will no longer be valid, feel free to remove or refactor them.

P.S. I've converted this PR to draft, until it is ready to review.

@frostming frostming marked this pull request as draft October 17, 2022 01:42
@frostming frostming marked this pull request as ready for review October 21, 2022 00:20
tests/cli/test_list.py Outdated Show resolved Hide resolved
@JohnHardy
Copy link
Contributor Author

Added a few of comments on the code where I need some help/advice about what the correct behaviour should be. Thanks!

src/pdm/cli/commands/list.py Outdated Show resolved Hide resolved
src/pdm/cli/commands/list.py Outdated Show resolved Hide resolved
tests/cli/test_list.py Outdated Show resolved Hide resolved
# # matches the environment or hashes\nAdd '-v' to see the detailed traceback\n"
# @pytest.mark.usefixtures("working_set", "repository")
# def test_list_bare_sorted_version_resolve(project, invoke, repository):
# # project.environment.python_requires = PySpecSet(">=3.6")
Copy link
Collaborator

@frostming frostming Oct 24, 2022

Choose a reason for hiding this comment

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

You need to mock the .metadata(or .prepare_metadata()) cuz it will be accessed during the command, which will try to find a package link and fail.

resolved_set = set(
c.prepare(project.environment).metadata for c in candidates.values()
)

@frostming frostming changed the base branch from main to feat/licenses October 28, 2022 02:10
@frostming
Copy link
Collaborator

@JohnHardy Thanks for the contribution, it may still need to be refined. I will merge it first and continue the work on another branch.

@frostming frostming merged commit 1c9286a into pdm-project:feat/licenses Oct 28, 2022
@JohnHardy
Copy link
Contributor Author

Fantastic, thank you for making last refinements.

I look forward to seeing the changes - please ping me if I can help in the future👍

@JohnHardy JohnHardy deleted the licenses branch October 28, 2022 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⭐ enhancement Improvements for existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants