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 CONTRIBUTING.md #16511

Merged
merged 1 commit into from
Jan 8, 2016
Merged

Update CONTRIBUTING.md #16511

merged 1 commit into from
Jan 8, 2016

Conversation

adidalal
Copy link
Contributor

@adidalal adidalal commented Jan 7, 2016

First pass at #16373

Comments welcome. cc @caskroom/maintainers

brew install vitorgalvao/tinyscripts/cask-repair
cask-repair --help

# use to update "cask-to-update"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

could use a better "placeholder name" here

Copy link
Member

Choose a reason for hiding this comment

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

No issues with the name, but the quotes took me a second. Something like <cask-to-update> {{cask-to-update}} might be clearer. If you’re unhappy with the name, how about outdated-cask?

@adidalal
Copy link
Contributor Author

adidalal commented Jan 7, 2016

Using cask-repair covers #16464 - not sure where else to mention appcast updates, as the rest is really about new casks.

brew install vitorgalvao/tinyscripts/cask-repair
cask-repair --help

# use to update <outdated-cask>
Copy link
Member

Choose a reason for hiding this comment

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

Since you’re using $github_user on the command, why not follow the same pattern for the cask? I.e.:

outdated_cask=<the-cask-i-want-to-update>
cd "$(brew --repository)/Library/Taps/caskroom/homebrew-cask"
cask-repair --pull origin --push $github_user $outdated-cask

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done and squashed

@vitorgalvao vitorgalvao added the documentation Issue regarding documentation. label Jan 7, 2016

```bash
# install and setup script - only needed once
brew install vitorgalvao/tinyscripts/cask-repair
Copy link
Member

Choose a reason for hiding this comment

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

Just renamed the repo to vitorgalvao/tiny-scripts (I wanted that name from the start, but at the time homebrew didn’t allow hyphens in tap names). The old name still works (looks like github is handling the redirection well), but still the new name should be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just "updated" my tap. All seems to be good.

[~] > brew untap vitorgalvao/tinyscripts
[~] > brew tap vitorgalvao/homebrew-tiny-scripts
[~] > brew uninstall cask-repair prfixmaster fastmerge
Uninstalling /usr/local/Cellar/cask-repair/0.13.3... (4 files, 11.5K)
Uninstalling /usr/local/Cellar/prfixmaster/0.4... (4 files, 7.4K)
Uninstalling /usr/local/Cellar/fastmerge/0.7.1... (4 files, 7.2K)
[~] > brew install cask-repair prfixmaster fastmerge
==> Installing cask-repair from vitorgalvao/tiny-scripts
==> Cloning https://github.com/vitorgalvao/tiny-scripts.git
{snip)

The Formulas repo needs a README update, but otherwise okay

Copy link
Member

Choose a reason for hiding this comment

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

The Formulas repo needs a README update

Nice catch. Thank you.

No need to reinstall, though, I’ve tried it, and homebrew seemed to still identify them correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CONTRIBUTING updated. If it looks good, we can merge this

@vitorgalvao vitorgalvao mentioned this pull request Jan 7, 2016
8 tasks
@adidalal
Copy link
Contributor Author

adidalal commented Jan 7, 2016

@zmwangx Here's the updated documentation with cask-repair mentioned. Let me know if things seem clear enough here

@zmwangx
Copy link
Contributor

zmwangx commented Jan 7, 2016

👍 LGTM, except one minor thing: cask-repair can't necessarily handle all casks (correct me if I'm wrong; I'm thinking about casks with funny download URLs that not only depends on version — there used to be such casks if memory serves), so maybe a disclaimer should be added.

@vitorgalvao
Copy link
Member

@zmwangx cask-repair does handle all casks (I started working on it before we started doing version interpolation). In the case where a cask has a non-interpolated URL, you’ll still type the version, but you’ll need the new URL in your clipboard (it automatically detects those cases and adjusts the message).

Remove extra sections
Add Updating Cask section
Revise style guide
@adidalal
Copy link
Contributor Author

adidalal commented Jan 7, 2016

@vitorgalvao I added a (slight) disclaimer - though cask-repair works almost all the time.

Let me know if the wording is too strong, either way.

@zmwangx
Copy link
Contributor

zmwangx commented Jan 7, 2016

Cool 👍 Just to make sure, are there any half interpolated URLs, e.g., https://example.com/2016/01/07/foo-#{version}.dmg, which would trick cask-repair to think of it as an always up-to-date URL? I think I've seen something like that, but it might be my illusion, or might be in a PR.

@vitorgalvao
Copy link
Member

I added a (slight) disclaimer - though cask-repair works almost all the time.

I’m fine with that. But any case cask-repair doesn’t work on I consider a bug (and fix when it is reported).

Also (and to also reply to)

Just to make sure, are there any half interpolated URLs, e.g., https://example.com/2016/01'07/foo-#{version}.dmg, which would trick cask-repair to think of it as a always up-to-date URL? I think I've seen so something like that, but it might be my illusion, or might be in a PR.

Lets not forget cask-repair does allow you to make manual fixes, and even rechecks and audits those for you.

So yes, @zmwangx, cask-repair will be tricked by those, but a half-interpolated URL isn’t useful anyway (i.e. it’s a fault of the cask, not cask-repair), and should be fixed in the cask itself (and you can do so within cask-repair).

@zmwangx
Copy link
Contributor

zmwangx commented Jan 7, 2016

should be fixed in the cask itself

Yes.

adidalal pushed a commit that referenced this pull request Jan 8, 2016
@adidalal adidalal merged commit 0ac17a4 into Homebrew:master Jan 8, 2016
@adidalal adidalal deleted the update-docs branch January 8, 2016 01:53
@miccal miccal removed the documentation Issue regarding documentation. label Dec 23, 2016
@Homebrew Homebrew locked and limited conversation to collaborators May 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants