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

Non-exhaustive fragment selection #115

Open
Jayshua opened this issue Feb 26, 2019 · 3 comments
Open

Non-exhaustive fragment selection #115

Jayshua opened this issue Feb 26, 2019 · 3 comments

Comments

@Jayshua
Copy link
Collaborator

Jayshua commented Feb 26, 2019

Selecting from interfaces is much easier than it used to be. Thanks for that! Curious if you might be interested in having an api like this as well?

type alias User =
    { id : Int
    , firstName : String
    , lastName : String
    , companyName : Maybe String
    }

userSelection : SelectionSet User Api.Interface.Account
userSelection =
    SelectionSet.map4 User
        Api.Interface.Account.id
        Api.Interface.Account.firstName
        Api.Interface.Account.lastName
        (Api.Interface.Account.onAccountCompany Api.Object.AccountCompany.name)

I went to write a selection for this interface and this is the first thing that came to mind. Which suggests this might be a good way for it to work.

I was thinking this would be in addition to the exhaustive fragment selections rather than instead of - automatic unwrapping of a maybe is pretty useful when selecting exhaustively.

Here's what it looks like right now for reference:

userSelection : SelectionSet User Api.Interface.Account
userSelection =
    let
        maybeFragments =
            Api.Interface.Account.maybeFragments
    in
    SelectionSet.map4 User
        Api.Interface.Account.id
        Api.Interface.Account.firstName
        Api.Interface.Account.lastName
        (Api.Interface.Account.fragments { maybeFragments | onAccountCompany = SelectionSet.map Just Api.Object.AccountCompany.name })
@dillonkearns
Copy link
Owner

Hey @Jayshua, thanks for opening the issue! The only downside I see is that it could confuse users to have more than one way to do the same thing. For example, someone might find the onAccountCompany function and hack something together to try to combine multiple fragments together. I like the way the code looks in your more concise version, but I wonder if the cost in API complexity/potential user confusion could outweigh the benefits here.

I wish that there was a more concise way to do a record update! Wouldn't it be nice if there was a way to pass in a record update as an argument! (Analogous to getters with .recordAttribute). If that were the case, then you could abstract away the default maybeFragments into the helper function, and write something like this:

Api.Interface.Account.fragments { ?| onAccountCompany = SelectionSet.map Just Api.Object.AccountCompany.name }

Actually, this would also make it easier to do optional arguments!

  Query.search (\optionals -> { optionals | first = Present 10 }) -- ...

Becomes:

  Query.search { ?| first = Present 10 } -- ...

And of course allowing { Api.Interface.Account.maybeFragments | foo = ...} to be used to update the record would at least marginally improve things by removing the let. This is being tracked, see issue 537 in this list elm/compiler#1375.

Anyway, those are my initial thoughts on that! What do you think? And thanks again for starting the discussion and sharing those code snippets, I appreciate it 😄

@Jayshua
Copy link
Collaborator Author

Jayshua commented Feb 28, 2019

It's a trade-off all right. The worse kind too, where it isn't totally clear which option is better. Here are a few more arguments in favor:

  • They aren't really doing the same thing. One is intended to grab the single interface variant you're interested in. The other is intended to "case" on all possible variants. If you're familiar with Rust it's like the difference between the match expression and the if let expression. They're similar, but one is intended to match on all possible variants, and the other on just the one variant you're interested in.

  • It could be viewed as an example of making the simple case easy and the general case possible. The HTTP library has special cases for making GET and POST requests, and lets the user drop down to a more complex API if they need more verbs. This is kind of similar.

  • If someone discovers the onAccountCompany function, I find it kind of difficult to believe they wouldn't also find the fragments function. How would they discover onAccountCompany? Intellisense? fragments would be next to it. Same for documentation pages. I could maybe see it if they are just reading existing code, but kind of seems like inventing a problem that doesn't exist. I could be wrong though.

I tried to come up with some arguments against, but I honestly couldn't think of anything that you didn't cover. 😅

Regarding the update expression, I'm personally in favor of the !firstName : a -> { b | firstName : a } -> { b | firstName : a } version, where !firstName is analogous to the .firstName getter. I'm pretty sure they could be composed too: !firstName "Alexander" >> !lastName "Clark". Just seems more in the functional tradition. This is probably another one of those "worse kind of trade-offs."

Definitely agree on the expressions in the record update syntax. I was really hoping that one would make it to 0.19. 😢

@dillonkearns
Copy link
Owner

Hello @Jayshua, thanks for the conversation, this is a very helpful discussion 👍

I've got some other plates in the air right now. I'll spend some time chewing on this in the meantime and play around with it a bit when I have some cycles. I think it's very much worth considering, I'll get a better sense of the tradeoffs when I get a chance to generate an API for it by hand and get a feel from using that.

Thank you so much for sharing your thoughts on this!

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