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

Update description #65

Merged
merged 2 commits into from
Dec 5, 2019
Merged

Update description #65

merged 2 commits into from
Dec 5, 2019

Conversation

ethomson
Copy link
Contributor

@ethomson ethomson commented Nov 8, 2019

No description provided.

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'
Copy link
Contributor

@thboop thboop Nov 20, 2019

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

Copy link
Contributor

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

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 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.

Copy link
Contributor

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

Copy link
Contributor Author

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)'
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

@ethomson ethomson force-pushed the ethomson/update_description branch from 55b8b1a to 6c7fa08 Compare December 5, 2019 04:14
@ericsciple ericsciple merged commit 7990b10 into master Dec 5, 2019
@ericsciple ericsciple deleted the ethomson/update_description branch December 5, 2019 13:50
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.

None yet

3 participants