-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
Fix minor input issues #25
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor clarification suggestions, what do you think?
README.md
Outdated
@@ -17,7 +17,7 @@ Every argument is optional. | |||
| Input | Description | Default | | |||
|--------------------|-----------------------------------------------------------------------------------------------------------------------------------|---------| | |||
| `package` | The package whose API to check for semver (in Package Id Specification format, see https://doc.rust-lang.org/cargo/reference/pkgid-spec.html for reference). If not set, all packages in the workspace are processed. | | | |||
| `manifest-path` | Path to Cargo.toml of crate or workspace to check. | | | |||
| `manifest-path` | Path to Cargo.toml of crate or workspace to check. By default, the action will assume it exists in the current directory. | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Current directory" is a tricky concept since we're talking about a GitHub Action invoked from a YAML file, not via the shell.
Perhaps:
| `manifest-path` | Path to Cargo.toml of crate or workspace to check. By default, the action will assume it exists in the current directory. | | | |
| `manifest-path` | Path to Cargo.toml of crate or workspace to check. By default, the action assumes the manifest is at the repo root. | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, current directory might not be very clear indeed, but is "repo root" really better? If I've read "repo root", I'd ask myself "which repo?" Not the repository checked for semver violations, as it might be cloned somewhere else, and not the repository with the action... In fact at the beginning there is no repo. What we mean here is some kind of "current directory", but in terms of the context in which the action is run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, good point.
How about "default GitHub Actions directory for the project" or something to that effect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a bit of googling I found out about GITHUB_WORKSPACE
variable, which according to https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables is "The default working directory on the runner for steps, and the default location of your repository when using the checkout action." which is exactly what we want to describe.
What do you think about saying that the action "assumes the manifest is under GITHUB_WORKSPACE
path" and linking https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables? Or maybe just saying that it is "the default working directory on the runner for steps" is better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like "assumes the manifest is under the default GITHUB_WORKSPACE
path" + the link you found. Ship it 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed!
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
As mentioned in #21.