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

Improve License Clarity at Top Package Level #3792

Draft
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

swastkk
Copy link
Collaborator

@swastkk swastkk commented Jun 1, 2024

Fixes #3802

Tasks

  • Reviewed contribution guidelines
  • PR is descriptively titled 📑 and links the original issue above 🔗
  • Commits are in uniquely-named feature branch and has no merge conflicts 📁
  • Updated documentation pages (if applicable)
  • Updated CHANGELOG.rst (if applicable)
  • Tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR
    Run tests locally to check for errors.

Copy link
Member

@AyanSinhaMahapatra AyanSinhaMahapatra left a comment

Choose a reason for hiding this comment

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

@swastkk you are regenerating all the tests with useless churn. This adds more things to review and is not ideal. Let's do things differently. You can delete your last commit and push regenerated test fixtures for only the tests which were failing.

@AyanSinhaMahapatra
Copy link
Member

@swastkk this is not correct atm:

  1. why use license_clarity and not license_clarity_score? This is what we use on the summery option.
  2. We want this new attribute license_clarity_score added to top-level packages if and only if the --package-summary option is used, and not in every package like you have here.

This new plugin could be there in packagedcode/plugin_package.py possibly, as we need to check if this option is enabled or not in process_codebase step for the package plugin, and then this should be passed on below to package.to_dict() fucntion for the same.

Copy link
Member

@AyanSinhaMahapatra AyanSinhaMahapatra left a comment

Choose a reason for hiding this comment

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

Does this make sure the license_clarity_score attribute is added only on using the --summary-package command line option? No. Look at your test regenerations, none of these tests have this option enabled, still have this attribute added.

You have to pass the package_summary option like we have the package_only CLI option here: https://github.com/nexB/scancode-toolkit/blob/develop/src/packagedcode/plugin_package.py#L203, then further pass it down to create_package_and_deps at https://github.com/nexB/scancode-toolkit/blob/develop/src/packagedcode/plugin_package.py#L263 and further to package.to_dict() at https://github.com/nexB/scancode-toolkit/blob/develop/src/packagedcode/plugin_package.py#L367 to actually be able to correctly set the attribute package_summary at https://github.com/swastkk/scancode-toolkit/blob/improve-license-clarity/src/packagedcode/models.py#L1544 you added thorugh this PR. Otherwise it's always set to one value.

Copy link
Member

@AyanSinhaMahapatra AyanSinhaMahapatra left a comment

Choose a reason for hiding this comment

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

More changes required.
Please merge latest develop afterwards.

tests/packagedcode/data/plugin/help.txt Show resolved Hide resolved
@@ -0,0 +1,6 @@
# Copyright Example Corp
Copy link
Member

Choose a reason for hiding this comment

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

Use real examples instead of synthetic ones.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can I use that change-case-5.4.4 example here as it has 4 packages inside it and can be more helpful while we have calculated the license_clarity_score since till now we just want to check if the plugin is working?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have added this test case, please have a look, otherwise we can remove this as well.

src/packagedcode/plugin_package.py Outdated Show resolved Hide resolved
src/packagedcode/plugin_package.py Outdated Show resolved Hide resolved
src/packagedcode/models.py Outdated Show resolved Hide resolved
src/packagedcode/models.py Outdated Show resolved Hide resolved
tests/scancode/data/help/help.txt Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Post Scan option --package-summary
2 participants