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

Use classes to access context #690

Closed
wants to merge 1 commit into from

Conversation

phadej
Copy link
Contributor

@phadej phadej commented Jan 30, 2017

As an example what I mean in #689: We don't need to force which construct is used as context. Users should be free to pick their own "extensible-records" library (if they want to use one).

@phadej phadej added the rfc label Jan 30, 2017
@jkarni
Copy link
Member

jkarni commented Jan 30, 2017

I don't mind the proposal, but I don't also see any huge gain. This is hardly the sort of access pattern that really requires sophisticated record libs (you just construct the datatype once, and never again modify it)

(Presumably we don't want to actually depend on lens just for this?)

@phadej
Copy link
Contributor Author

phadej commented Jan 30, 2017

lens: well we don't need to. There wasn't setters at all previously. The only lost feature is Inject, but I have hard time to come up with generally useful combinator requiring it.

The gain: it's much more approachable (especially if we don't use lens, but only getter functions) to people who aren't comfortable with HLists. And for them who are, we don't force them to use our implementation.

At work I construct Ctx with connections pools etc, which I partially apply to handlers (maybe I should use enter runReaderTNat, but anyway). So to get e.g auth working, i'd just write an instance HasAuthCheck Ctx and pass it to serveWithContext...

The enter runReaderTNat and serverWithContext context solve about the same problem, but quite differently. I'm not sure if MonadReader context (Handler context) (with context :: * as here) is a good idea, but maybe it is.

TL;DR it's hard to balance between "simplicity of implementation" and "not much boilerplate", but for that reason lens has makeLenses for example. We could have too deriveDefaultServantHasClasses.

@jkarni
Copy link
Member

jkarni commented Jan 30, 2017

Hmm, interesting. On the one hand I think configuring handlers and combinators are distinct enough to be kept as separate notions, but on the other I do see how it'd be useful if the same datatype could be used for both so that one ends up with a single configuration datatype.

I'm overall coming over to this idea, but still have a question. Let's say we have a default config - the one one serve uses. We have a datatype for it, and instances for all the classes our base combinator instances need. If there's one more combinator that needs something else in the context, it seems like doing that all of a sudden gets a lot harder. Previously you'd just prepend the new data. Now you need to declare a new datatype, and a whole lot of new instances. This is solved decently well by makeClassy, but now we're back to using lens (or microlens).

@phadej
Copy link
Contributor Author

phadej commented Jan 31, 2017

@jkarni: we could still provide hlist-Context, at least for some time. And/or write own TH method for generating boilerplate: if it's as simple "derive servant built-in classes, except ones in this list", it's very trivial.

So it would be better to have only getters, as then () could be the default.

@phadej
Copy link
Contributor Author

phadej commented Feb 1, 2017

I'll improve this after 0.10 is out. Showing code will probably help.

@alpmestan
Copy link
Contributor

I have a few ideas in that space I'd like us to experiment with, so I agree that we should postpone this to after 0.10.

@phadej phadej closed this Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants