Skip to content

Fixed false positive occuring in issue #4022 #4034

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
Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fixed false positive occuring in issue #4022 #4034

wants to merge 1 commit into from
+39 −1

Conversation

aayushkdev
Copy link
Contributor

@aayushkdev aayushkdev commented Dec 21, 2024 •
edited
Loading

Fixes #4022

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 📁
  • Updated documentation pages (if applicable)
  • Updated CHANGELOG.rst (if applicable)

AyanSinhaMahapatra
AyanSinhaMahapatra requested changes Jan 14, 2025
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.

@lyr-ast thanks for your PR!
Could you check #4022 (comment), we need a couple more new rules here to close this issue.

@aayushkdev
Copy link
Contributor Author

Yeah, sure! I've just pushed the changes. Please check if there are any other issues.

@aayushkdev aayushkdev closed this Jan 14, 2025
@aayushkdev aayushkdev reopened this Jan 14, 2025
@aayushkdev aayushkdev force-pushed the issue-#4022 branch 10 times, most recently from b4a7b61 to ae90f4b Compare January 17, 2025 22:18
@aayushkdev
Copy link
Contributor Author

hey @AyanSinhaMahapatra just a friendly ping, could you check if this pr is ready to merge

AyanSinhaMahapatra
AyanSinhaMahapatra requested changes Apr 16, 2025
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.

@aayushkdev see changes in required phrases for rules, thanks!

src/licensedcode/data/rules/mit_1354.RULE Outdated
- "Jia PengHui"
---

MIT License
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
MIT License
{{MIT License}}

src/licensedcode/data/rules/mit_1354.RULE Outdated

MIT License

Copyright (c) {{2016 Jia PengHui}}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Copyright (c) {{2016 Jia PengHui}}
Copyright (c) 2016 Jia PengHui

src/licensedcode/data/rules/mit_1352.RULE Outdated
relevance: 100
---

This library is licensed under The {{MIT}}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This library is licensed under The {{MIT}}
This library is {{licensed under The MIT}}

@aayushkdev aayushkdev force-pushed the issue-#4022 branch 3 times, most recently from 5f73c94 to ce10f6b Compare April 16, 2025 18:52
@aayushkdev
Copy link
Contributor Author

aayushkdev commented Apr 16, 2025 •
edited
Loading

Hey @AyanSinhaMahapatra thanks for the re-review I have made the changes you requested and i clearly have a lot to learn about rules in scancode-toolkit.

Also some of the tests fail which seem to be unrelated to the changes I made

pombredanne
pombredanne reviewed Apr 21, 2025
View reviewed changes
src/licensedcode/data/rules/mit_1353.RULE Outdated
minimum_coverage: 80
---

his library is licensed under The {{MIT License}}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
his library is licensed under The {{MIT License}}
This library is licensed under The {{MIT License}}

pombredanne
pombredanne reviewed Apr 21, 2025
View reviewed changes
src/licensedcode/data/rules/mit_1354.RULE Outdated

{{MIT License}}

Copyright (c) 2016 Jia PengHui
Copy link
Member

Choose a reason for hiding this comment

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

No copyrights in rule please ... unless absolutely needed. This is not needed here

@aayushkdev
Copy link
Contributor Author

aayushkdev commented Apr 26, 2025 •
edited
Loading

Hey @pombredanne the mit_1353.RULE is a duplicate rule of mit_750.RULE which also caused many testcases to fail so I have removed it from the pr.

Also removing the copyright Copyright (c) 2016 Jia PengHui from the mit_1354.RULE causes the failure of test_license_match_unknown_license_intro_with_imperfect_matches and test_parse_with_unpacked_wheel_meta_v23_1 testcase which is the result of the testcase expecting mit.LICENSE but instead getting mit_1354.RULE as the rule_identifier
would you like me to modify the testcases too?

@aayushkdev
Fixed False Positive detection for amazon-sl license by adding rules …
dc6177a 
…for mit license and updating rules for amazon-sl license. issue aboutcode-org#4022

Signed-off-by: Aayush Kumar <aayush214.kumar@gmail.com>
@aayushkdev aayushkdev requested a review from pombredanne April 26, 2025 15:30
@pombredanne
Copy link
Member

@aayushkdev yes, please update tests accordingly.

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 Awaiting requested review from pombredanne

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

Successfully merging this pull request may close these issues.

False Positive: Incorrect detection of amazon-sl license expression
3 participants
@aayushkdev @pombredanne @AyanSinhaMahapatra