-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Update description #65
Conversation
action.yml
Outdated
@@ -1,10 +1,10 @@ | |||
name: 'Checkout' | |||
description: 'Checkout a Git repository.' | |||
description: 'Clone and checkout a Git repository at a particular version' |
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.
By default, we checkout what we think is the right reference for the user's build (a merge commit on pr's, the head of a the branch on pushes, ect). I think this is misleading and we should just say
Clone and checkout a git repository
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.
@ethomson i pushed changes to master for checkout v2 (beta right now). It doesnt do a full fetch anymore, now it just fetches the single commit (improved perf for mainline scenario)
Curious if you think your proposed description still fits? I'll defer to your judgement
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 think that just saying "checkout a git repository" - without any additional information
- suggests to the reader that it will be at the remote HEAD. Indicating that it will be "at a particular version" hints that a) it might not be the repository's default and b) it might be able to be overridden.
Not tied to this verbiage.
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.
Thanks makes sense. It was the "Clone" part of your proposed changes that I was worried about. The phrase "at a particular version" is good. Let me know if you want to update the PR otherwise i can make the changes. There is an npm run gendocs
command that will regen the input descriptions in the README.md from the action.yml
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.
Cool. I pushed an update with some minor tweaks. TBQH the descriptions are already good so this is super minor.
action.yml
Outdated
inputs: | ||
repository: | ||
description: 'Repository name' | ||
ref: | ||
description: 'Ref to checkout (SHA, branch, tag)' | ||
description: 'Reference or ID to checkout (SHA, branch, tag)' |
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.
The branch, SHA or tag to checkout
might be a little less verbose.
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.
either way seems good. Reference and ID aligns with git terminology, and the name of the input
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.
If users can specify just a branch name here (eg, master
) then I think that saying "Branch" up-front is ok. But if it needs to be in proper reference format (eg, refs/heads/master
) then I think that we should say "Reference or ID" up front.
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.
master
and refs/heads/master
are both supported
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.
Nice. In that case I agree with @thboop. Will push an update.
55b8b1a
to
6c7fa08
Compare
No description provided.