Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add AlpinePackages pipeline #272
Add AlpinePackages pipeline #272
Changes from all commits
be62961
ee6d8a2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the
self.aports_dir_path
variable really needed?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since
image_id
is not used, I would suggest:for alpine_version in self.alpine_versions.values()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
items()
was @pombredanne 's commit suggestion.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@quepop values() would be better since you do not use the
image_id
variable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following code should be made more digest and readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, when the name of the keyword argument and the provided variable is the same, it's explicit enough to only keep the variable.
For example:
I think the following is as explicit and more readable:
It make sense to keep the keyword agrs in the following example though:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used unnamed positional arguments before and @pombredanne commented that i should use named positionals everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with the "makes reading much easier" in the cases mentioned above but "more resistant to refactorings" may be a fair point.
You can leave it as-is then ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could store that call in a
unscanned_packages
to help with the for loop layout.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This
run_extractcode
function does not exists in the main branch anymore since b035f00. You need to migrate to the newscancode.extract_archives
API.See https://github.com/nexB/scancode.io/blob/main/scanpipe/pipelines/scan_codebase.py#L73 for an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to call directly the ScanCode
scancode.api.get_copyrights
function instead of starting a full scancode subprocess.This will be more efficient and will remove the need for
extract_summary_fields
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need a copy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we have to download and extract the sources somewhere and it shouldn't be inside of the aports repo directory. Furthermore doing a scan on a single directory is in my opinion much better. If we were to do two separate scans, path handling and scan result merging would make the code much less clean. Also we have the files in one place for further investigation (if something is wrong with the package) and it simply would not be possible if we didn't copy them because we do checkouts in a loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so if I understand correctly you are:
I think it could be better if you separate each operation and the process could benefit from more documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly.
If i separated 1. from 2. it would have only
copytree(apkbuild_dir, scan_target_path)
as the function body. Also, 3. is already separate from the rest.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So do you want me to split it? Given my reasons above I don't think we should. Also when we look at the rest of the scancode.io source files there are many instances where much bigger functions than mine aren't split into smaller ones.