Prepare new release - #33
Merged
Merged
Conversation
* This is handy for windows to have the same path as linux Signed-off-by: Chin Yeung Li <tli@nexb.com>
Create junction from Scripts to bin
Signed-off-by: Jono Yang <jyang@nexb.com>
Check for deps in local thirdparty directory #31
Signed-off-by: Jono Yang <jyang@nexb.com>
Signed-off-by: Jono Yang <jyang@nexb.com>
* Create copyright statement from holder information Signed-off-by: Jono Yang <jyang@nexb.com>
* This is used for the case where we are starting off a project and have not yet generated requirements files Signed-off-by: Jono Yang <jyang@nexb.com>
Signed-off-by: Jono Yang <jyang@nexb.com>
* Replace all references to `tmp` with `venv` Signed-off-by: Jono Yang <jyang@nexb.com>
* Add --init option to configure.bat
* Update help text in configure and configure.bat
Signed-off-by: Jono Yang <jyang@nexb.com>
Signed-off-by: Jono Yang <jyang@nexb.com>
Signed-off-by: Jono Yang <jyang@nexb.com>
* Update README.rst Signed-off-by: Jono Yang <jyang@nexb.com>
Signed-off-by: Jono Yang <jyang@nexb.com>
* Update README.rst with instructions for post-initialization usage Signed-off-by: Jono Yang <jyang@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
* Replace references to scancode-toolkit repo with links to the skeleton repo
* Remove --python option from configure.bat
Signed-off-by: Jono Yang <jyang@nexb.com>
Update skeleton
Signed-off-by: Jono Yang <jyang@nexb.com>
Add README.rst to etc/scripts/
Signed-off-by: Chin Yeung Li <tli@nexb.com>
Fixed #41 - Handled encoding issue when generating ABOUT files
And not a possible binaries Also Ensure that we craft a minimally parsable license expression, even if not correct. Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
The upload is otherwise shaky. Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
These archive should not crash extraction when using --replace-originals Reported-by: Smascer @Smascer Reported-by: Bryan Sutula @sutula Reference: #31 Reference: aboutcode-org/scancode-toolkit#2723 Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
These archive should not crash extraction when using --replace-originals Reported-by: Smascer @Smascer Reported-by: Bryan Sutula @sutula Reference: #31 Reference: aboutcode-org/scancode-toolkit#2723 Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
sutula
reviewed
Oct 8, 2021
sutula
left a comment
There was a problem hiding this comment.
First, this change does fix issue #2723. Thank you!
Looking over the entire function, and please keep in mind I don't know this codebase at all, I observe the following:
- New line 129 and following will log an error message that is specific to "replace_originals", yet it seems that the log message is not within an "if replace_originals" code block, so it could potentially log a misleading or at least distracting message?
- Looking at the entire flow of the original code, and considering the two cases, "replace_originals" true and "replace_originals" false, if we are not replacing originals we loop on only "yield event". All other code is inside an "if replace_originals" block. In the other case of replace_originals true, we do the same "yield event", as well as queue events which we will later process. Now the original code seems straightforward. However the patch, in particular the "if event.warnings or event.errors" feels like it should be inside the following "if replace_originals". It works either way, I guess, but it seems confusing to test for an error condition, potentially logging a message specific to that condition, in code that will be executed when replace_originals is false.
I think my comments above are long-winded. The suggestion here is to move line 136 up under line 128, indenting the patch (new lines 129-135) by one more level.
This is cleaner than to do tis in the main loop. Also refine and format the documentation. Reported-by: Bryan Sutula @sutula Reference: #31 Reference: aboutcode-org/scancode-toolkit#2723 Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Member
Author
|
@sutula Thank you for your review. I cleaned up the code in the latest commit based on your feedback, |
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
THere was a dangling JSON file loaded for no reason Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
The new skeleton now use venv and not tmp Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Member
Author
|
The Ubuntu 16 failures are a a side-effect of Azure issues. Not ours. Merging |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This PR fixes a few issues in particular issues with
the --replace-originals option