examples: Add comments with sample run commands #123
Conversation
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
There was a problem hiding this comment.
Looks great, @CatalinStratu! I've left a few comments that probably ought to be fixed before this is merged. If you come up against issues when force-pushing to your fork (in order to update the pull request), please let me know and I'll be happy to help :)
| @@ -8,7 +8,7 @@ | |||
| // This is generally only useful when run with two SPDX documents that | |||
| // describe licenses for subsequent versions of the same set of files, AND if | |||
| // they have the same identifier in both documents. | |||
|
|
|||
| // Run project: go run example_licensediff.go ../sample-docs/example1/spdx/example1.spdx ../sample-docs/example2/spdx/example2-src.spdx | |||
There was a problem hiding this comment.
The files at those two paths don't seem to exist - are they from a previous version and now longer present?
I changed those to ../sample-docs/example1/spdx/SPDXTagExample-v2.2.spdx and ../sample-docs/example2/spdx/SPDXTagExample2-v2.2.spdx respectively, but got the following error:
Successfully loaded first SPDX file ../sample-docs/example1/spdx/SPDXTagExample-v2.2.spdx
Error while parsing ../sample-docs/example2/spdx/SPDXTagExample2-v2.2.spdx: received unknown tag SnippetSPDXID in Package section/project/examples/6-licensediff
I don't think the error is related to this pull request, since SnippetSPDXID is a valid tag anyway. There must be an issue in ../sample-docs/example2/spdx/SPDXTagExample2-v2.2.spdx or in the tooling.
There was a problem hiding this comment.
@seabass-labrax This is probably a bug, both files are equal.
There was a problem hiding this comment.
This does appear to be a bug in either the tvloader parser or else possible the example file. I see that @CatalinStratu filed #124 so we can take a look there.
@CatalinStratu please note though that the files in example1/spdx/... and example2/spdx/... are not actually the same. They have the same filename, but take a look and you'll see that the contents are different. We can dig into this further in #124.
| _ = fmt.Errorf("%v", ok) | ||
| return |
There was a problem hiding this comment.
| _ = fmt.Errorf("%v", ok) | |
| return | |
| fmt.Println(fmt.Errorf("%v", ok)) | |
| os.Exit(1) |
I like the exception-style error handling, but it's not quite complete yet. fmt.Errorf won't actually print the error itself, so it needs to printed with another function call. I'd also suggest using os.Exit(1) so that the exit status is correctly printed.
| _ = fmt.Errorf("error opening File: %s", err) | ||
| return |
There was a problem hiding this comment.
| _ = fmt.Errorf("error opening File: %s", err) | |
| return | |
| fmt.Println(fmt.Errorf("error opening File: %s", err)) | |
| os.Exit(1) |
Same as above.
There was a problem hiding this comment.
It's a good idea, I made the necessary changes. Thanks!
| #### Run project: * go run example_report.go ../sample-docs/example1/spdx/SPDXTagExample-v2.2.spdx | ||
| * |
There was a problem hiding this comment.
| #### Run project: * go run example_report.go ../sample-docs/example1/spdx/SPDXTagExample-v2.2.spdx | |
| * | |
| #### Run project: * go run example_report.go ../sample-docs/example1/spdx/SPDXTagExample-v2.2.spdx * |
I think this is a stray asterisk 😃
There was a problem hiding this comment.
The README.md file no longer exists.
There was a problem hiding this comment.
@seabass-labrax is correct, this is in the main examples/README.md file so this does need a fix. I'll update separately after merging this PR.
|
Hi @CatalinStratu, thanks for this! I haven't had a chance to review in detail yet (or to look at @seabass-labrax's feedback either), but one quick note: Please take another look at my comment at #120 (comment). I mentioned that the files to go into an Can you please take another look, and revise so that this is just adding the SPDX documents and nothing else for the |
Signed-off-by: CatalinStratu <catalinstratu45@gmail.com>
I made the necessary changes. Please review it. |
|
Thank you @CatalinStratu! There are a couple of subsequent tweaks but I can address them in a subsequent PR. I'm going to squash these commits since it looks like the submitted ones have the extra README files getting added and then deleted. |
swinslow
changed the title
Hi @swinslow , I have added the necessary changes mentioned in #120