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

Allow customizing the application symlink #2532

Merged
merged 1 commit into from
Jan 25, 2014

Conversation

voanhduy1512
Copy link
Contributor

An enhancement to support #1415

@rolandwalker
Copy link
Contributor

👍 on both the feature and the interface. I've tested this.

The Travis build will start working cleanly when you change the respond_to? typo noted on your commit.

In the documentation, we should note that this is a general feature, not just for link. From the point of view of a Cask author, it works for every artifact type except install. For instance, you can do

  font :source => 'Symbola.ttf',
       :target => 'AnotherName.ttf'

Also on the docs - if Cask authors start using the feature, we will probably have to explain somewhere that even though an App is installed under a different name, the Bundle ID inside the App bundle is unchanged. When a user has two apps installed with the same Bundle ID, they can sometimes get unexpected behavior.

@voanhduy1512
Copy link
Contributor Author

About link (...), this PR works fine without the parentheses when link have one args (or one source and one target). But if link have multi agrs, like Alfred, i can't make it work without the square brackets. Because link expect to have a set of agrs, so i with square brackets to specific an array here.

# with parentheses
link([{:source => 'Alfred 2.app', :target => 'AnotherName.app'}, 'Alfred 2.app/Contents/Preferences/Alfred Preferences.app'])
#  without parentheses
link [{:source => 'Alfred 2.app', :target => 'AnotherName.app'}, 'Alfred 2.app/Contents/Preferences/Alfred Preferences.app']

So with multi agrs link, the agrs must be an array with each item is a hash or a string. I dont know how to write all these things into the document in an easy way. Maybe you should help me with the document here

@ghost ghost assigned rolandwalker Jan 22, 2014
@rolandwalker
Copy link
Contributor

That's funny, when I tried, only () worked but not []. I'll test again.

If docs are not easy, let's do that the opposite way: I'll write the doc in a separate pull request, and you can comment on my PR until you are happy with it.

Please notice I have added tests to support your work in #2543.

@muescha
Copy link
Contributor

muescha commented Jan 23, 2014

How about if i like to give my custom name for each cask while install

Like:

brew cask install firefox "My Firefox"

@phinze
Copy link
Contributor

phinze commented Jan 23, 2014

Thanks for your work on this @voanhduy1512!

I'd like to propose a slight modification to the interface here and see what y'all think.

Nearly all Cask Artifacts have been designed with the idea that you can specify them multiple times and they are merged together.

class MyApp < Cask
  # ...
  link 'One.app'
  link 'Two.app'
end

For a few artifact types, we've also been able to compose by specifying multiple arguments. It looks like for link the multiple-argument strategy has been what people have ended up using. (There are nine Casks with >1 arg for link).

I think we might be able to end up with a simpler interface if we drop support for >1 link arg and have the first arg always reference the source.

Example from #1415:

class ScalaIde < Cask
  # ...
  link 'eclipse/Eclipse.app', :target => 'Scala IDE.app'
end

Multiple links with targets:

class MyAppCanary < Cask
  # ...
  link 'One.app', :target => 'One Canary.app'
  link 'Two.app', :target => 'Two Canary.app'
end

This avoids all the ([{ type syntax needed for multiple links and I think simplifies things overall. What do you think?

rolandwalker added a commit to rolandwalker/homebrew-cask that referenced this pull request Jan 24, 2014
- no functional changes in this patch
- preparation for interface change allowing custom
  target links in Homebrew#2532
@voanhduy1512
Copy link
Contributor Author

Thanks for your comment and suggestions. I will modify the PR

@voanhduy1512
Copy link
Contributor Author

Sorry, wrong click :D

@voanhduy1512
Copy link
Contributor Author

@phinze I like the idea in your modified interface but i have some difficult to make it work.

link 'One.app', :target => 'One Canary.app'

This will make like have 2 arguments, one string and one hash, not just 1 hash. And this

link 'One.app', :target => 'One Canary.app'
link 'Two.app', :target => 'Two Canary.app'

will make link have 4 arguments. And I can't determine the order of them, sometimes the :target go first, sometime don't, set#merge make it too confused to determine. So to make it work like you suggest, i have to modified the dsl parser (and that too risky because it make to modify too large, may affect other artifact and your planning for them).
After checking on some artifact, i realised that their agrument is always a String or a Hash, not mix of Hash and String. So i suggest to keep the source key to make it clear and same format as other artifact.

class MyAppCanary < Cask
  # ...
  link :source => 'One.app', :target => 'One Canary.app'
  link :source => 'Two.app', :target => 'Two Canary.app'
end

A little longer but easy to make it work (in fact is done with the current state of this PR) and no modified to other part of the project

@voanhduy1512
Copy link
Contributor Author

thanks for @rolandwalker advices, i think i can make the interface like @phinze suggestion, wait for my PR

rolandwalker added a commit to rolandwalker/homebrew-cask that referenced this pull request Jan 25, 2014
- no functional changes in this patch
- preparation for interface change allowing custom
  target links in Homebrew#2532
rolandwalker added a commit to rolandwalker/homebrew-cask that referenced this pull request Jan 25, 2014
Travis won't like this until/if Homebrew#2532 is merged.
rolandwalker added a commit that referenced this pull request Jan 25, 2014
Allow customizing the application symlink
@rolandwalker rolandwalker merged commit c59ba39 into Homebrew:master Jan 25, 2014
rolandwalker added a commit that referenced this pull request Jan 25, 2014
tests for #2532 - customizing target symlink
rolandwalker added a commit to rolandwalker/homebrew-cask that referenced this pull request Jan 25, 2014
rolandwalker added a commit to rolandwalker/homebrew-cask that referenced this pull request Feb 12, 2014
This follows up Homebrew#2532 and Homebrew#2392 abstracting out some duplicated
code.  Still not perfect, but better.
@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.

4 participants