Skip to content
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

Provide an example for check command #162

Merged
merged 3 commits into from
Oct 24, 2022

Conversation

davidhsingyuchen
Copy link
Contributor

@davidhsingyuchen davidhsingyuchen commented Oct 23, 2022

Summary

PR fixes #160 (comment).

Notes

  • The usage of **Tip** comes from

    **Tip**: go-licenses writes the report to stdout and info/warnings/errors logs

  • --include_test is also added because that's the very reason why I created Add include_tests flag for deps only used in tests #160, and Add include_tests flag for deps only used in tests #160 (comment) also mentions:

    I would use the flag when running check, because all deps must be allowed.

  • I thought about putting this section after those of the flags (e.g., --disallowed_types), but I feel that go-licenses check --include_tests github.com/your-org/your-repo/... is potentially a template command that suits most use cases (i.e., users only need to replace the placeholders to make the command work for them), so I put it right after the intro of check. No strong opinion though.

@@ -218,7 +218,16 @@ go-licenses save <package> [package...] --save_path=<save_path>
Checking for forbidden and unknown licenses usage:

```shell
go-licenses check <package> [package...]
go-licenses check <package> [package...]
Copy link
Contributor Author

@davidhsingyuchen davidhsingyuchen Oct 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the trailing space. Github is not showing it probably because this line belongs to the same chunk as the modified lines below.

README.md Outdated Show resolved Hide resolved
README.md Outdated

**Tip**: Usually you'll want to add

* `...` to check all the packages in your repository
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make it clear add ... to package

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, but the repository root is not necessarily an importable package.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I understand your comment, as your linked doc ... means a pattern to match packages, root does not need to be a package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought you meant that I should change to "... to package (i.e., github.com/google/go-licenses) to check all the packages in your repository", but maybe I misunderstood your point.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh, I was not clear. let me clarify. I think this sentence lacks some context. It does not tell where to add ...

so I am asking you to make the sentence more complete. Add description that ... is appended to the end of an import path to include all packages matching that pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the clarification! Fixed in 1d973cb.

Copy link
Collaborator

@Bobgy Bobgy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@Bobgy Bobgy merged commit e3924ce into google:master Oct 24, 2022
@davidhsingyuchen davidhsingyuchen deleted the add-check-example branch October 24, 2022 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants