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

Add DSL for after_install and similar blocks #5723

Merged
merged 1 commit into from
Aug 12, 2014

Conversation

federicobond
Copy link
Contributor

Refs #5268

This is just a prototype version. Some tests are broken and others missing, but I wanted to get feedback on the general direction and style advice so that I can continue working.


module InstalledMethods
def info_plist
"#{destination_path}/#{artifacts[:link].first.first}/Contents/Info.plist"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

artifacts[:link].first.first is ugly. Any better way?

Copy link
Contributor

Choose a reason for hiding this comment

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

In Lisp there is caar. Ruby is somewhat Lispy, but seems to lack this.

@rolandwalker
Copy link
Contributor

For a prototype, this is pretty great! The functionality would definitely get used by many Cask authors. Also, we can be fairly liberal about merging something incomplete as long as it stays undocumented.

My main question is the choice of class name/filename. Will another coder realize that decorator.rb is the place to look for this?

@federicobond
Copy link
Contributor Author

Yes, I am not a fan of the names either. Cask::DSL::AfterInstall sounds much better, but Cask::DSL is a module that gets included into Cask so it would be confusing to use Cask::DSL::AfterInstall as a decorator.

@federicobond
Copy link
Contributor Author

Two other points:

  • I am not sure system_command the best name for the public interface method. cmd, run are other alternatives.
  • Enabling access for assistive devices involves the after_install and before_uninstall blocks to do things properly. Adding a stanza that builds on top of this DSL may be cleaner than having to use both methods in every cask.

@federicobond federicobond changed the title [wip] Add DSL for after_install and similar blocks Add DSL for after_install and similar blocks Aug 11, 2014
@federicobond
Copy link
Contributor Author

Tests are passing. There is certainly room for improvement but, if the other maintainers agree, we can merge it as is. Let me know and I can squash the commits.

@rolandwalker
Copy link
Contributor

Let's squash and merge. Momentum is important.

Later – must it be considered a decorator? The method_missing here is elegant, but also has some undesired/surprising effects. An artifacts method is exposed to Cask authors, which is not wanted. Nor is it wanted to set version, only read it. In caveats.rb we take a different approach, with a constrained list of methods.

include Cask::DSL::Installed

def suppress_move_to_applications
system_command("/usr/bin/defaults", :args => ["write", bundle_identifier, "suppressMoveToApplications", "-bool", "true"])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out suppressMoveToApplications is not a standard at all, and most of the Casks use moveToApplicationsFolderAlertSuppress instead. There is also kNTMoveToApplicationsFolderAlertSuppress.

We may also need a generic DSL method for accessing defaults.

@federicobond
Copy link
Contributor Author

@rolandwalker PR is ready to merge. Thanks for the feedback.

I like the approach used by caveats.rb. I will try to submit a PR later to get rid of the decorator.

federicobond added a commit that referenced this pull request Aug 12, 2014
Add DSL for after_install and similar blocks
@federicobond federicobond merged commit ef71905 into Homebrew:master Aug 12, 2014
@federicobond federicobond deleted the postflight-dsl branch August 12, 2014 01:07
@rolandwalker
Copy link
Contributor

It's great to have your input. I'm closing #5268 and #5266. I tried to remove the assistive_devices method from caveats.rb, but there are a couple of Casks using it. We will have to wait until this is in release.

WRT assistive devices needing to be explicit on install and uninstall, perhaps we can leverage the upcoming .metadata directory to stow some directives to be used at uninstall time.

@rolandwalker
Copy link
Contributor

Since it is taking so many weeks to finish #4688, that means this code has been out for a long time, and these elements can be blessed into DSL 1.0. No objections, right?

I only want to look at the question of consistency for calling external commands from a Cask. Currently, I have the problem that installer_script and uninstall :script have annoyingly inconsistent interface. I am going to try to reconcile them, and look at your system_command here as well.

@federicobond
Copy link
Contributor Author

No objections. Let me know if there are any pressing issues you need help with. I've been a little out of the loop lately.

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

2 participants