Improve License Clarity at Top Package Level

[swastkk]

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 📁
• Tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR
• Updated documentation pages (if applicable)
• Updated CHANGELOG.rst (if applicable)
  Run tests locally to check for errors.

Signed-off-by: swastkk swastkk@gmail.com

[AyanSinhaMahapatra]

@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]

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

[AyanSinhaMahapatra]

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.

[AyanSinhaMahapatra]

More changes required.
Please merge latest develop afterwards.

[AyanSinhaMahapatra]

See comments above. And sup on populating license clarity and other attributes?

[AyanSinhaMahapatra]

Couple more nits.

[AyanSinhaMahapatra]

See initial comments: We need some major refactoring.

Also please do not include multiple empty files for test when only one is fine. For example you have multiple empty files under tests/packagedcode/data/package_summary/change-case-change-case-5.4.4.zip-extract/change-case-change-case-5.4.4/packages/change-case/src/. Also add some license expressions in the source files (not all files, only one) to check whether other_license_expression is correctly populated.

[AyanSinhaMahapatra]

Suggested change

	for resource in resources.walk(topdown=True):
	for resource in codebase.walk(topdown=True):

This is wrong and extremely confusing code. You cannot store a codebase object in a resources variable and walk/get attributes from it depending on a flag value. You should only pass resources here from before, and these should always be resource objects consistently. See other comments on this.

[AyanSinhaMahapatra]

Let's not make this function complex, we should instead, do a codebase.get_resource(path=resource_path) to get the resource objects for a specific path string (make sure you test these for the strip_root CLI option).

We should not do two things which are basically the same, depending om a flag value, but we should try to make the inputs same instead, so that same code is valid for both.

[AyanSinhaMahapatra]

This is True everywhere else, you are using resources to carry both codebase objects and resource dictionaries. This is never a good thing, make sure you only carry a list of resource objects and the codebase object seperately (some cases this is needed for cetain functions) in both cases.

[swastkk]

By this, I didn't understand why we will be doing the codebase.get_resource(path=resource_path), as we are using a single func compute_license_score and inside it we were calling the get_field_values_from_resources which can be renamed, as for the package-summary, we were passing the Complete Package with all its resources comibined together with resource field, shouldn't it be a better approach to use a flag for getting field values for the Package Object that already contains resources with it and normal field_value collecttion from the complete codebase for the overall summary for the codebase.

Like using package_level_summary flag in the compute_license_score func or something else?

[AyanSinhaMahapatra]

understand why we will be doing the codebase.get_resource(path=resource_path)

Because in one case we are using a resource mapping, in another case we are using resource objects, this should not be the case, it should always be obejcts.

See also the following comment:

you are using resources to carry both codebase objects and resource dictionaries.

This is not okay. We should instead modify and always use a list of resources (for a specific package, or alternatively all the resources in the codebase) passed in the function.