Add include_tests flag for deps only used in tests#160
Conversation
|
Thank you! |
|
Oh, I'm sorry for being offline for so long. It's great that @davidhsingyuchen picked up my old work and fell free to base on them if they can help. I'm totally consent with this @Bobgy . |
|
@davidhsingyuchen can you fix the test? |
|
@Bobgy Thanks for the reminder. |
Bobgy
left a comment
There was a problem hiding this comment.
Perfect!
Thank you for the great contribution!
Would you mind also update README with its documentation?
|
@Bobgy Thank you for the review! I try to follow the other sections in README to explain why this flag may be useful. Please let me know if the tone sounds good to you, thanks! |
README.md
Outdated
| ### Include testing packages | ||
|
|
||
| Use the `--include_tests` global flag to include packages only imported by testing code. | ||
| For example, those packages could be testing libraries/frameworks. |
There was a problem hiding this comment.
For example, testing libraries or frameworks.
There was a problem hiding this comment.
That's not a complete sentence though. I merged the two sentences to make it more concise.
README.md
Outdated
| Use the `--include_tests` global flag to include packages only imported by testing code. | ||
| For example, those packages could be testing libraries/frameworks. | ||
| Usually you would want to add this flag because if you're already analyzing the licenses of your dependencies, | ||
| likely those packages also need to be taken into account. |
There was a problem hiding this comment.
probably omit this?
you described what effect this flag has, I think it is reasonable to let users decide whether they need the flag.
I would use the flag when running check, because all deps must be allowed.
when saving report for a binary, I won't use it, because testing libraries are not included.
There was a problem hiding this comment.
Good point, updated.
README.md
Outdated
| Example command: | ||
|
|
||
| ```shell | ||
| go-licenses check --include_tests "github.com/google/go-licenses" |
There was a problem hiding this comment.
Can you try github.com/google/go-licenses/... ?
Just checking the root package may not include all deps used. It seems more reasonable to check all pkgs.
There was a problem hiding this comment.
Just checking the root package may not include all deps used. It seems more reasonable to check all pkgs.
Makes sense. Probably this is also worth mentioning in ### Check? If it sounds good to you, I can create a follow-up PR to update that to make this PR focused.
Bobgy
left a comment
There was a problem hiding this comment.
Awesome! Thank you again!
PR fixes #62. PR is based on the work of @blanet.