Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Move options to DSL #9982

Closed
adamv opened this Issue · 27 comments

6 participants

@adamv
Owner

def options was modelled after def patches, but returning lists of lists is kind of weird.

Suggest that we deprecate the def options syntax and replace it with:

option 'name', 'description'

Well-known symbols may also be supported, such as:

option :universal

A new build_options member will be available on Formula, and using this object will be suggested instead of inspecting ARGV directly.

By default, an option will be a boolean flag, true if the option was passed, false if not. We will also add support for valued options.

@mistydemeo
Owner

I thought it would be a good idea to implement options access in formula logic via accessors, like Imagemagick is doing manually now.

E.g., boolean options could be accessed with a foo? accessor that returns true or false (for --use-foo or --with-foo style options). Non-boolean options could be accessed with a foo accessor that returns the specified value (for --with-foo=bar style options).

@adamv
Owner

Agreed; updated the main description.

@adamv adamv was assigned
@jacknagel
Owner

Agree with everything here.

@Sharpie
Collaborator

Agreed as well. I have often thought that def options should somehow provide a shortcut that saves developers from manually mapping common flags like --with-foo to tests like ARGV.foo?.

@adamv
Owner

I'm not sold on automagically creating method names out of option names, but I won't discount the idea. Though I made leave it for someone else to implement that part.

@adamv
Owner

Style question: should option descriptions end in a period or not?

option 'zsh-completion', "Install zsh completions."

vs.

option 'zsh-completion', "Install zsh completions"

Not a huge deal, but consistency looks professional.

@adamv
Owner

Running into a slight issue.

Take this formula:

class Audiofile < Formula

  depends_on 'lcov' if ARGV.include? '--with-lcov'

  def options
    [['--with-lcov', 'Enable Code Coverage support using lcov.']]
  end
end

The depends_on is dependent on build options, and is declared first.
This sort of thing is somewhat common, and means we can't use auto-generated method names to do conditional dependencies.

@jacknagel
Owner

Style question: should option descriptions end in a period or not?

I'd say omit the period.

@jacknagel
Owner

Running into a slight issue.

Maybe you can glean some ideas from https://github.com/mxcl/brew2/blob/master/share/brew/formula/foo.rb (or maybe not).

@mikemcquaid
Owner

I vote a period at the end. Any reason for audiofile you couldn't put the depends after the options instead?

@adamv
Owner

(A) I'm putting periods at the ends, so there. (B) I'll revisit this issue when I get further into the code. I have a working options DSL but haven't tested all the edge cases.

@adamv
Owner

Rebased and pushed; added build.stable?

@adamv
Owner

Rebased: https://github.com/adamv/homebrew/tree/ops

Still need to update brew audit to know about the new options scheme.

@samueljohn

Is this ready to test (and still applies to master), @adamv?

@adamv
Owner

Thing I don't like: you still have to do if ARGV.build_head? in the DSL because build.head? isn't defined yet.

@adamv
Owner

Pushed a change to make build available to the DSL. Need someone better at ruby to doublecheck my work.

@adamv
Owner

Reposted; simplified access to build; make a copy of ARGV (or any args array).

@jacknagel
Owner

I left some comments on adamv/homebrew@de9f511 a few days ago

@adamv
Owner

Always surprising where GitHub surfaces commit comments or not.

@adamv
Owner

Updated with those two suggestions

@adamv
Owner

Rebased on top of last week's options change.

@adamv
Owner

Another review requested; would like to merge this week if no problems.

@adamv
Owner

Now defaulting description to "" if missing. Anything else?

@jacknagel
Owner

:thumbsup:

@adamv adamv closed this issue from a commit
@adamv adamv Add `option` to the DSL
Closes #9982
402bd72
@adamv adamv closed this in 402bd72
@matbd matbd referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@shrop shrop referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@Sharpie Sharpie referenced this issue from a commit in Sharpie/homebrew
@adamv adamv Add `option` to the DSL
Closes #9982
c4ed62b
@Sharpie Sharpie referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@fgeller fgeller referenced this issue from a commit in fgeller/homebrew
@adamv adamv Add `option` to the DSL
Closes #9982
823cf13
@fgeller fgeller referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@Nexuapex Nexuapex referenced this issue from a commit
@adamv adamv Add `option` to the DSL
Closes #9982
f0aa9b1
@ckdaas ckdaas referenced this issue from a commit
@adamv adamv Add `option` to the DSL
Closes #9982
ad44352
@snakeyroc3 snakeyroc3 referenced this issue from a commit in snakeyroc3/homebrew
@adamv adamv Add `option` to the DSL
Closes #9982
e313510
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.