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.

Sign up for GitHub

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

Jump to bottom

Report unknown licenses separately. #2578

Closed
wants to merge 4 commits into from
Closed

Report unknown licenses separately. #2578

wants to merge 4 commits into from

Conversation

akugarg
Copy link
Collaborator

@akugarg akugarg commented Jun 28, 2021

Signed-off-by: akugarg akanksha.garg2k@gmail.com

Fixes #2574

Tasks

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

AyanSinhaMahapatra
AyanSinhaMahapatra requested changes Jun 28, 2021
View reviewed changes
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.

Btw, we have to create a resource attribute like this here, and you have to return unknown_licenses here.

src/scancode/api.py Outdated

detected_licenses.extend(
_licenses_data_from_match(
if "unknown" in match.rule.license_expression:
Copy link
Member

Choose a reason for hiding this comment

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

matches will have a is_unknown flag you added :P Use that instead of checking the license_eexpression.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yeah! Actually that PR is not yet merged therefore changes were not reflected in this branch which was giving errors, that's why I have used this .

pombredanne
pombredanne reviewed Jun 29, 2021
View reviewed changes
src/scancode/api.py Outdated Show resolved Hide resolved
@akugarg
Create a separate section for unknown licenses
5851f1f 
Signed-off-by: akugarg <akanksha.garg2k@gmail.com>
@akugarg akugarg force-pushed the Report_licenses_separately branch from 9222562 to 5851f1f Compare June 30, 2021 05:28
AyanSinhaMahapatra
AyanSinhaMahapatra requested changes Jun 30, 2021
View reviewed changes
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.

Should fix the tests next.

src/scancode/api.py Show resolved Hide resolved
src/scancode/api.py Show resolved Hide resolved
src/licensedcode/plugin_license.py Outdated Show resolved Hide resolved
src/scancode/api.py Show resolved Hide resolved
@akugarg
Create a separate section for unknown licenses
decb959 
Signed-off-by: akugarg <akanksha.garg2k@gmail.com>
@akugarg akugarg force-pushed the Report_licenses_separately branch 2 times, most recently from 9e91a14 to d514cbb Compare July 3, 2021 12:07
@akugarg
Update test cases upto expectation
ccf9de4 
Signed-off-by: akugarg <akanksha.garg2k@gmail.com>
@akugarg akugarg force-pushed the Report_licenses_separately branch from d514cbb to ccf9de4 Compare July 5, 2021 04:45
@AyanSinhaMahapatra
Copy link
Member

Some tests are failing in summarycode/plugin_consolidate.py. See here. Another normalized_unknown_license_expression should be saved to each child, and then added to Consolidation.other_license_expression. And related test expectations fixed.

@akugarg akugarg force-pushed the Report_licenses_separately branch from b26a578 to 412ea31 Compare July 7, 2021 06:58
@akugarg akugarg force-pushed the Report_licenses_separately branch from 412ea31 to 908e8a4 Compare July 7, 2021 07:59
AyanSinhaMahapatra
AyanSinhaMahapatra reviewed Jul 7, 2021
View reviewed changes
src/summarycode/plugin_consolidate.py Outdated
@@ -404,6 +412,7 @@ def create_consolidated_components(resource, codebase, holder_key):

c = Consolidation(
core_license_expression=combine_expressions(license_expressions),
other_license_expression = combine_expressions(unknown_expressions),
Copy link
Member

Choose a reason for hiding this comment

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

No spaces should be present before and after = in this case.

pombredanne
pombredanne reviewed Jul 11, 2021
View reviewed changes
src/scancode/api.py Outdated
_licenses_data_from_match(
match=match,
include_text=include_text,
license_text_diagnostics=license_text_diagnostics,
license_url_template=license_url_template)
)


if "unknown" in match.rule.license_expression: #TODO: use is_unknown flag instead
Copy link
Member

Choose a reason for hiding this comment

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

re #TODO: use is_unknown flag instead when would this happen?

Copy link
Member

Choose a reason for hiding this comment

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

@pombredanne after #2548 is approved and merged, then this could be rebased/merged and the is_unknown flags introduced there can be used.

pombredanne
pombredanne requested changes Jul 11, 2021
View reviewed changes
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. See my comments inline.

src/scancode/api.py Outdated Show resolved Hide resolved
tests/cluecode/data/plugin_filter_clues/filtered-expected2.json Outdated Show resolved Hide resolved
tests/cluecode/data/plugin_filter_clues/filtered-expected3.json Outdated
@@ -33,12 +33,12 @@
"sha256": "408dfe8f9a70ea7bc9feaa9f77fb731ada0c26dc39008fdc5ead52d0442c656f",
"mime_type": "text/plain",
"file_type": "ASCII text",
"programming_language": null,
"programming_language": "verilog",
Copy link
Member

Choose a reason for hiding this comment

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

This is weird... how is this related to the current PR?

tests/formattedcode/data/json/tree/expected.json Show resolved Hide resolved
@akugarg akugarg force-pushed the Report_licenses_separately branch from 33634cf to 7c22012 Compare July 11, 2021 19:36
@akugarg
Update test cases upto expectation
8f93fda 
Signed-off-by: akugarg <akanksha.garg2k@gmail.com>
@akugarg akugarg force-pushed the Report_licenses_separately branch from 7c22012 to 8f93fda Compare July 12, 2021 06:02
@pombredanne
Copy link
Member

@akugarg @AyanSinhaMahapatra should we merge this? or is this no longer relevant based on our latest discussions?

@AyanSinhaMahapatra
Copy link
Member

@pombredanne Yes, this is not relevant anymore. @akugarg Thanks though.

@pombredanne pombredanne closed this Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AyanSinhaMahapatra AyanSinhaMahapatra AyanSinhaMahapatra requested changes

@pombredanne pombredanne pombredanne requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unknown Licenses should be reported separately
3 participants
@akugarg @AyanSinhaMahapatra @pombredanne