Prepare new release #33
Merged
Conversation
This file contains 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
* This is handy for windows to have the same path as linux Signed-off-by: Chin Yeung Li <[email protected]>
Create junction from Scripts to bin
Signed-off-by: Jono Yang <[email protected]>
Check for deps in local thirdparty directory #31
Signed-off-by: Jono Yang <[email protected]>
Signed-off-by: Jono Yang <[email protected]>
* Create copyright statement from holder information Signed-off-by: Jono Yang <[email protected]>
* 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 <[email protected]>
Signed-off-by: Jono Yang <[email protected]>
* Replace all references to `tmp` with `venv` Signed-off-by: Jono Yang <[email protected]>
* Add --init option to configure.bat
* Update help text in configure and configure.bat
Signed-off-by: Jono Yang <[email protected]>
Signed-off-by: Jono Yang <[email protected]>
Signed-off-by: Jono Yang <[email protected]>
* Update README.rst Signed-off-by: Jono Yang <[email protected]>
Signed-off-by: Jono Yang <[email protected]>
* Update README.rst with instructions for post-initialization usage Signed-off-by: Jono Yang <[email protected]>
Signed-off-by: Philippe Ombredanne <[email protected]>
* Replace references to scancode-toolkit repo with links to the skeleton repo
* Remove --python option from configure.bat
Signed-off-by: Jono Yang <[email protected]>
Update skeleton
Signed-off-by: Jono Yang <[email protected]>
Add README.rst to etc/scripts/
Signed-off-by: Chin Yeung Li <[email protected]>
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 <[email protected]>
The upload is otherwise shaky. Signed-off-by: Philippe Ombredanne <[email protected]>
Signed-off-by: Philippe Ombredanne <[email protected]>
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 <[email protected]>
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 <[email protected]>
sutula
reviewed
Oct 8, 2021
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 <[email protected]>
|
@sutula Thank you for your review. I cleaned up the code in the latest commit based on your feedback, |
Signed-off-by: Philippe Ombredanne <[email protected]>
Signed-off-by: Philippe Ombredanne <[email protected]>
Signed-off-by: Philippe Ombredanne <[email protected]>
Signed-off-by: Philippe Ombredanne <[email protected]>
THere was a dangling JSON file loaded for no reason Signed-off-by: Philippe Ombredanne <[email protected]>
The new skeleton now use venv and not tmp Signed-off-by: Philippe Ombredanne <[email protected]>
Signed-off-by: Philippe Ombredanne <[email protected]>
|
The Ubuntu 16 failures are a a side-effect of Azure issues. Not ours. Merging |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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