Add is_notice flag to the --classify option #3822 #4142
Conversation
aayushkdev
force-pushed
the
3822-add-is_notice-flag-classify
branch
from
49e93f0 to
a9f7ed3
Compare
aayushkdev
force-pushed
the
3822-add-is_notice-flag-classify
branch
5 times, most recently
from
ba6181f to
f8eed11
Compare
There was a problem hiding this comment.
Thanks++ @aayushkdev see review comments for your consideration, this is looking great otherwise.
| @@ -64,6 +64,17 @@ def test_set_classification_flags_is_package_data_file(self): | |||
| set_classification_flags(res) | |||
| assert res.is_manifest | |||
|
|
|||
|
|
|||
| def test_set_classification_flags_is_notice(self): | |||
| test_dir = self.get_test_loc('classify/notice') | |||
There was a problem hiding this comment.
Can you also add another test for package_NOTICE in an example here as mentioned in #3822 (comment) (please always read the original issue and all the comments there carefully for more details), I don't think we are getting this logic correctly yet here. This could also be an issue in how we determine the is_legal flag, nevertheless it would be nice to also address this in the PR.
There was a problem hiding this comment.
Yeah, I’ve added package_NOTICE.txt in the test, but just a quick note: the comment example used the Resource class, which caused check_resource_name_start_and_end to return false. However, the is_legal and is_notice checks use ScannedResource from the Codebase class, which works as expected and returns true for notice files with an extension, the tests that I have added also work for notice files with an extension.
That said, if I’m mistaken in my approach, I’m happy to go ahead and add the suggested tests just need a clarification on this.
src/summarycode/tallies.py
Outdated
| @@ -334,7 +334,7 @@ def tally_codebase_key_files(codebase, field='tallies', **kwargs): | |||
| # filter to get only key files | |||
| key_files = (res for res in codebase.walk(topdown=True) | |||
| if (res.is_file and res.is_top_level | |||
| and (res.is_readme or res.is_legal or res.is_manifest))) | |||
| and (res.is_readme or res.is_legal or res.is_manifest or res.is_notice))) | |||
There was a problem hiding this comment.
Is this required? doesn't is_legal includes is_notice too?
There was a problem hiding this comment.
You're right I have removed it, that was an oversight from me
| @@ -1934,6 +1934,7 @@ files: | |||
| is_readme: no | |||
| is_top_level: yes | |||
| is_key_file: no | |||
| is_notice: no | |||
There was a problem hiding this comment.
I could not find any example of is_notice: True in any of the tests here. Could you add a NOTICE file somewhere in the full scancode scan tests so we can test for this?
There was a problem hiding this comment.
I might be mistaken, but from what I understand, the formattedcode tests are designed to scan only code files, not key files. and since there are no existing test cases for key files within formattedcode, I have not added a test case there.
but I have added a new test for notice in multiple_package_data in summarycode as there was no testcase there for notice
aayushkdev
force-pushed
the
3822-add-is_notice-flag-classify
branch
2 times, most recently
from
1e892b7 to
fa3961e
Compare
Signed-off-by: Aayush Kumar <[email protected]>
aayushkdev
force-pushed
the
3822-add-is_notice-flag-classify
branch
from
fa3961e to
a7b5827
Compare
|
Hey @AyanSinhaMahapatra these failed tests pass on my system I don't think their failing has to do anything with the pr. also please let me know if there are any other changes you would like me to make in the pr. |
Fixes #3822
Added
is_noticeflag to--classifyfor detecting package notice filesThe
--classifyoption now includes anis_noticeflag to help identify package notice files. Currently, detection is based only on file names.Tasks
Run tests locally to check for errors.