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

Implement consistent pattern for options #2

Closed
rundis opened this issue Jan 26, 2017 · 5 comments
Closed

Implement consistent pattern for options #2

rundis opened this issue Jan 26, 2017 · 5 comments

Comments

@rundis
Copy link
Owner

rundis commented Jan 26, 2017

Options as List - Lists handling internally
Currently most widgets take options as a List of some Module encapsulated "Option" union type.
The implentation isn't great though because it also treats in as a List internally, because you can easily specify the same option twice (or however many). For some options that could lead to quite unexpected results. (havent tested what bootstrap would show if you wanted a btn-small and btn-large, but when possible we shouldn't invite this ambiguity

Options as List - Record internally
A perhaps somewhat better solution would be like the one used in the Progress module. Where the options is defined as a record internally.
You can still add an option several times, but the behavior is a bit more predictable or in our hands.

I've been thinking a lot on how I'd love to figure out a way that was even less ambiguous and still gives a nice user experience for the API. (I tried using records/type-aliases in the API, but it didn't turn out very nice. Maybe I just didn't find the appropriate pattern.

Thoughts on where to go with this one ?

@rundis
Copy link
Owner Author

rundis commented Jan 27, 2017

Given options are represented some like this for a Progress module

type Options msg
    = Options
        { value : Int
        , height : Maybe Int
        , label : List (Html msg)
        , role : Maybe Role
        , striped : Bool
        , animated : Bool
        , attributes : List (Attribute msg)
        }

Another option would be to provide a pipeline friendly API for providing options

Current:

Progress.progress 
    [ Progress.value 20, 
    , Progress.success, 
    , Progress.height 10 
    ]

Pipelined:

Progress2.progress
    (Progress2.defaultOptions
        |> Progress2.danger
        |> Progress2.animated
        |> Progress2.value 20
      )

-- or even (not as clear what's going on with the actual view being `progress` last in the chain
Progress2.defaultOptions
    |> Progress2.danger
    |> Progress2.animated
    |> Progress2.value 20
    |> Progress2.progress -- maybe view would be a better name ?

@rtfeldman, @mikeonslow thoughts ?

@mikeonslow
Copy link

mikeonslow commented Feb 1, 2017

@rundis Question: Since the pipeline version builds/transforms the options as it moves through the pipeline, how would duplicates be handled?

If you specified:

Progress.defaultOptions |> Progress.danger |> Progress.warning |> Progress.success |> Progress.progress

Which one would win? And if it's the last one in the pipeline, how is this different than

Progress.progress [ Progress.danger , Progress.warning , Progress.success ]

@rundis
Copy link
Owner Author

rundis commented Feb 1, 2017

To a user of the API they will appear very similar (although the function signatures would give more of a clue what's going on in the pipeline friendly alternative). If you update the same record field twice it will be the last one standing :-) Also with the pipeline-friendly record (although opaque) alternative I think it's slightly easier to grok that it's a finite set of options at disposal.

Another alternative might be to use records straight out:

progress
    { Progress.defaultOptions
        | role = Progress.success  
        , value = 1            
    }

... but of course that fails on Progress.defaultOptionsused inside the curly brackets since the parser checks for identifiers starting with a lowercase letter (: Ref elm/compiler#635 . None of the workarounds are very appealing.

And even if it were possible you could still do this

progress
    { Progress.defaultOptions
        | role = Progress.success  
        , value = 1            
        , role = Progress.danger
        , value = 5
    }

Summary
From an API user perspective the list approach and pipeline approach looks to convey pretty much the same semantics (or lack thereof !). These are the two best alternatives I've managed to come up with. They both have strengths and weaknesses and they both leave you yearning for something better. I'll try a few more things before resigning to this is the two alternatives we must decide on.

@rundis
Copy link
Owner Author

rundis commented Feb 3, 2017

One thing against the pipeline approach I just realized. Let's say that progress took a list of children as an argument as well. progress : Options msg -> List (Html msg) -> Html msg (this would be true for many of the other widgets/elements in the lib.

Then this way of writing the pipeline would obviously not work:

Progress2.defaultOptions
    |> Progress2.danger
    |> Progress2.animated
    |> Progress2.value 20
    |> Progress2.progress children  -- OUCH, we would have to change the order of argument !

So you'd have to introduce parens

Progress.progress
    ( Progress2.defaultOptions
        |> Progress2.danger
        |> Progress2.animated
        |> Progress2.value 20
    ) 
    [ text "Some child thing that doesn't make sense in this particular example" ]

@rundis
Copy link
Owner Author

rundis commented Feb 21, 2017

Went for the list based approach with pruning duplicates (last item wins) where applicable.

@rundis rundis closed this as completed Feb 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants