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

Create binary.md documentation #28822

Merged
merged 9 commits into from
Jan 10, 2017
Merged

Create binary.md documentation #28822

merged 9 commits into from
Jan 10, 2017

Conversation

miccal
Copy link
Member

@miccal miccal commented Jan 10, 2017

Closes #25423.

@miccal miccal added awaiting maintainer feedback Issue needs response from a maintainer. documentation Issue regarding documentation. labels Jan 10, 2017
@miccal miccal changed the title Create binary.md Create binary.md documentation Jan 10, 2017
@@ -0,0 +1,48 @@
# binary

In the simple case of a string argument to `binary`, the source file is linked into the `/usr/local/bin` folder on installation. For example (from [operadriver.rb](https://github.com/caskroom/homebrew-cask/blob/60531a2812005dd5f17dc92f3ce7419af3c5d019/Casks/operadriver.rb#L11)):
Copy link
Member

Choose a reason for hiding this comment

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

I think we usually call them “directories” not “folders”. Or maybe we’re inconsistent on that. I prefer the former.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

creates a symlink to:

```bash
/usr/local/bin/operadriver
Copy link
Member

@vitorgalvao vitorgalvao Jan 10, 2017

Choose a reason for hiding this comment

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

This isn’t strictly true. In practice we use $(brew --prefix)/bin. Most of the time that will translate to /user/local/bin, but not always. See #21372.

Fixing it in https://github.com/caskroom/homebrew-cask/blob/master/doc/cask_language_reference/all_stanzas.md (#28837).

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

/usr/local/bin/operadriver
```

from a source file such as:
Copy link
Member

Choose a reason for hiding this comment

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

I don’t think this section is necessary. It’s not that useful to know where it’s linked from, that’s a more advanced detail.

Copy link
Member Author

Choose a reason for hiding this comment

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

I followed the same language as used in app.md which says the same thing.

Happy to remove it, though.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove it from app.md as well. As long as it’s consistent, I’ll leave it to what you find is clearer.

Copy link
Member Author

@miccal miccal Jan 10, 2017

Choose a reason for hiding this comment

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

I think that it is better in the binary because it actually is a symlink, and users would like to know where the symlink points to.

It can be removed from app.md though, I think.

Copy link
Member

Choose a reason for hiding this comment

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

I think that it is better in the binary because it actually is a symlink, and users would like to know where the symlink points to.

It can be removed from app.md though, I think.

Agreed.


## Renaming the Target

You can rename the target which appears in your `/usr/local/bin` directory by adding a `target:` key to `binary`, for example to be to be consistent with other command-line tools like [changing case](https://github.com/caskroom/homebrew-cask/blob/070a3bdeadf339892268d79aded9108ba7960f32/Casks/praat.rb#L12):
Copy link
Member

Choose a reason for hiding this comment

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

We can name it “binaries directory” or something, here, instead of /usr/local/bin, since as previously noted, it might not be that location.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

We can name it “binaries directory” or something, here, instead of /usr/local/bin, since as previously noted, it might not be that location.

Fixed.

The examples could be taken directly from https://github.com/caskroom/homebrew-cask/blob/master/doc/cask_language_reference/stanzas/app.md#target-should-only-be-used-in-select-cases, for consistency.

That is where I got them from.

binary 'minishift-darwin-amd64', target: 'minishift'
```

## target: May Contain an Absolute Path
Copy link
Member

Choose a reason for hiding this comment

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

While useful, this part is referring to artifact, not binary. We don’t want to encourage absolute paths in binary, target:. The section should be removed or made clearer that it is referring to artifact as a companion to binary.

Copy link
Member Author

@miccal miccal Jan 10, 2017

Choose a reason for hiding this comment

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

Again, I followed the same language as used in app.md which says the same thing.

Happy to remove it, though.

Although the example is nice, as it is a binary example.

Might actually make more sense to remove this from app.md also and leave it in binary.md.

Copy link
Member

Choose a reason for hiding this comment

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

Lets do one better. I think that I was the one to originally write these, and that I put target in app.md for lack of a better place. We could either:

  • Split the documentation of target: to a new document and link there from both app.md and binary.md
  • Remove all target: documentation from binary.md and link to the section in app.md.

I’m partial to the second one (seems to be as clear, and requires less files.

Copy link
Member Author

@miccal miccal Jan 10, 2017

Choose a reason for hiding this comment

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

Sounds good to me. Please add the third example, though, about cleaning up the name.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm offline for a while - please fell free to edit the file.

Copy link
Member

@vitorgalvao vitorgalvao Jan 10, 2017

Choose a reason for hiding this comment

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

I’m instead removing mentions of binary.

In this document, we can say that for target:, the app.md document should be followed, but that the select cases don’t apply as rigidly and it’s fine to take some extra liberties with target: to be consistent with other command-line tools, like changing case, removing an extension, or cleaning up the name.

This way it’ll be more consistent with what we do with the other repos (we tell users to check the rules in this one, and point out the differences there).

Copy link
Member Author

Choose a reason for hiding this comment

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

I Agree.

@miccal miccal removed awaiting maintainer feedback Issue needs response from a maintainer. documentation Issue regarding documentation. labels Jan 10, 2017
@miccal miccal merged commit 334f331 into master Jan 10, 2017
@miccal miccal deleted the miccal-binary-doc branch January 10, 2017 23:53
@miccal
Copy link
Member Author

miccal commented Jan 11, 2017

Thanks for all the help @vitorgalvao.

@vitorgalvao
Copy link
Member

@miccal Thank you for starting it in the first place.

@Homebrew Homebrew locked and limited conversation to collaborators May 9, 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.

2 participants