Use of unexported `option` type makes it hard to wrap

[bradleypeabody]

The type option is unexported and yes necessary to use when calling libsass.New. I'm looking for a way around this - so far reflection seems like the only choice, but if I want to dynamically choose which set of options I am using, I don't seem to have any other choice than to explicitly make a big switch statement with all of my possible libsass.New() call variations, since I cannot make for example a slice of option.

I realize this was probably done following the pattern of some other library and in order to not export unnecessary types. But I do think this is not a great design choice. It makes it really hard to compose a custom set of options based on whatever logic (I'm trying to write a command line wrapper around this - here: https://github.com/vugu/vgsassc) and I don't think that was the intention.

Would you consider making option into Option ?

[drewwells]

The pattern is called functional options. It was coined by Dave Cheney. You can find a short explanation here https://github.com/tmrts/go-patterns/blob/master/idiom/functional-options.md

It was kept private so there’s no requirement to support backwards compatibility when changing it. Since it has not changed in quite a while, it could be made public.

https://godoc.org/github.com/wellington/go-libsass#BasePath I believe all the options are already configurable by these public methods. What are you trying to change on the options struct?

[drewwells]

Even if Option were public, it should not be mutable after instantiation from the outside. I'm thinking on ways to do this, but it's most likely going to require an API change.

type OptFunc func(o *libsass.Option)

libsass.NewWithOptions(io.Writer, io.Reader, OptFunc...)

Another way is to add an optional func that accepts OptFunc as an argument

func FuncOpt(func (o *libsass.Option)) option

It would be used like this

libsass.New(writer, reader, libsass.FuncOpt(func(o *libsass.Option) {
 o.builddir = somevalue
})

In either case, we'd probably add a new Option type that gets translated into the internal one. This would further enforce non-mutability of options needed for the compiler to work.

[bradleypeabody]

Cool - and yeah after that last message I think we're on the same page. Just to recap:

The issue is I cannot declare something like var options []libsass.option and then dynamically put together my own options so I can then do something like: c, err := libsass.New(out, in, options...)

I want to write something like:

    var options []libsass.option
    switch *oOutputStyle { // a command line flag
    case "nested":
        options = append(options, libsass.OutputStyle(libsass.NESTED_STYLE))
    case "expanded":
        options = append(options, libsass.OutputStyle(libsass.EXPANDED_STYLE))
    // ...
    }
    c, err := libsass.New(out, in, options...)

But instead I end up having to do:

    var newArgs = make([]reflect.Value, 0, 8)
    newArgs = append(newArgs, reflect.ValueOf(out))
    newArgs = append(newArgs, reflect.ValueOf(in))
    switch *oOutputStyle {
    case "nested":
        newArgs = append(newArgs, reflect.ValueOf(libsass.OutputStyle(libsass.NESTED_STYLE)))
    case "expanded":
        newArgs = append(newArgs, reflect.ValueOf(libsass.OutputStyle(libsass.EXPANDED_STYLE)))
    // ...
    }

    newRet := reflect.ValueOf(libsass.New).Call(newArgs)
    errV := newRet[1]
    err, ok := errV.Interface().(error)
    if ok {
        log.Fatal(err)
    }
    compV := newRet[0]
    c := compV.Interface().(libsass.Compiler)

Another possible solution: What if libsass declared:

type Options []option

I think this would be enough to make this work:

    var options libsass.Options
    options = append(options, libsass.OutputStyle(libsass.NESTED_STYLE))
    c, err := libsass.New(out, in, options...)

And seems to require no other refactoring.

[drewwells]

Ohhhh, that's a funny thing I did here. You don't actually need option type made public, you need the functions themselves to be public type. Maybe I can cheat this a little, I'll try some stuff.

New(dst, src, FuncOpt...)

Then you can do

var opts []FuncOpt
...
libsass.New(dst, src, opts...)

[bradleypeabody]

Thanks! This looks like it will work well, I'll try it out soon.