Servant.API.TypeLevel

[fizruk]

Closes #305.

I need to write some tests, otherwise, should be complete.

[jkarni]

The similar version in -foreign can be removed in favour of this one, I think.

[fizruk]

Done.

[jkarni]

Also fixes #121

[jkarni]

LGTM

[codedmart]

I don't know much about type families yet, but also LGTM. 👍

[aaronlevin]

LGTM 👍

[fizruk]

FYI: some more cool stuff is being embedded in servant-swagger.

[dredozubov]

The only issue i have with this is style: indentation is very inconsistent, sometimes i see 2 spaces, sometimes 4. Can i propose 2 spaces for the whole repo? We have to deal with huge type declarations and systematically having code closer to the left side will do some good IMHO.

[phadej]

shouldn't this be at the bottom of export list?

[dredozubov]

there'll be some warning if anything will be shadowed afaik

[soenkehahn]

@dredozubov: I don't fully understand your comment. What is shadowed by what here? In any case I would expect the order of the export list to be irrelevant.

[fizruk]

@dredozubov I am in favour of 2 spaces too.

It has also been brought to our attention that singletons implements a lot of type-level machinery in their Data.Promotion.Prelude modules. I am currently considering adding singletons as a dependency of servant to reuse these.

[dredozubov]

@fizruk I personally feel like adding singletons to the dependency list is like opening a gateway to hell. In my experience it quickly escalates to the longer compile-time and awkward(IMHO) type signatures, because "we already have singletons as a dep, why not do X?". To make picture fuller, there is also https://hackage.haskell.org/package/type-fun

[fizruk]

@dredozubov that is my only concern so far, yes.

[arianvp]

What is holding us back currently to merge this? Except from the discussion whether we want to add an extra dependency for type-level utility functions?

[fizruk]

@arianvp tests are still missing and there are also few things in Servant.Swagger.Internal.TypeLevel that we might want to move here too. But that can be done in a separate PR.

I am currently overloaded at work so I won't be able to work on this in the next couple of weeks.

[kosmikus]

Do I misunderstand this PR? This isn't adding new functionality, but moving scattered functions to a common module, right? If so, I don't think we should require new tests for this to go in. Also, adding additional functions from elsewhere should be a separate PR.

And I'd vote against adding a dependency on singletons if it can be avoided.

As far as this is just a refactoring (which I thought it was), I'm happy for it to be merged.

[fizruk]

@kosmikus there is new functionality here, e.g. Endpoints and IsSubAPI and a few other type families which came from (an older version of) Servant.Swagger.Internal.TypeLevel.API.

Over the time servant-swagger gained more type-level stuff (like BodyTypes). I had a few more type families in mind (e.g. ParamTypes to extract types for ToParamSchema testing), but never had time to add those.

My idea was to figure out all useful types in servant-swagger and then move them here.

[kosmikus]

I see. But then, why should all this be added to the servant package? I can kind of understand it for the things that are needed by the core servant machinery itself. But if it becomes more like "let's add all type families that anyone could ever need", it feels like it'd be better in a separate servant-typelevel package to me.

[fizruk]

@kosmikus well, maybe. But if we go with servant-typelevel then some things have to stay in servant (e.g. ones used by Servant.Utils.Links) while others move to servant-typelevel. There aren't that many type-level functions left, so I did not consider an option of servant-typelevel.

Most of the type-level code is actually very general and not related to servant per se. Hence the singletons discussion. servant-swagger has some general type-level code too. It is not clear where all this code belongs.

I also think type-level functions like IsSubAPI and BodyTypes are essential for servant-based documentation and testing:

• IsSubAPI (used in servant-swagger) is a "more general" version of IsIn used in servant-docs; it is very useful when modifying documentation for a selected part of an API;
• BodyTypes is useful to extract a list of all API types used for request/response bodies filtered by content-type; it enables things like Servant.Swagger.Test, but is not limited to that.

IMO, these are not just some type functions "anyone could ever need", but rather "anyone building a servant-something library should probably use". Think servant-docs, servant-quickcheck, etc.

In any case, feel free to kill this PR and introduce another one which would just put existing things in one place. We can discuss all the extra type-level functions in a separate issue/PR.

[kosmikus]

I definitely agree that there are probably a whole lot of useful type-level functions in the context of servant, and that it makes sense to provide them, rather than letting every user of servant reinvent them.

So for the time being, I'm happy to go with the approach proposed here. It just think we should keep an eye on how much we'll accumulate over time. As long as there are no new external dependencies (such as singletons) introduced by this effort, I think we're probably ok.

[jkarni]

I just rebased against master. @haskell-servant/maintainers can I merge? As far as I can tell we have two +1s from before, unless something changed.

[soenkehahn]

I'm happy to give this another review before merging.

[soenkehahn]

Whitespaces are inconsistent.

[jkarni]

Added loads of doctests and other documentation.

[soenkehahn]

Can we also squash the commits before merging, please?

[jkarni]

Yup. Got some more work to do first though.

On Thu, Aug 25, 2016 at 08:24:45AM -0700, Sönke Hahn wrote:

Can we also squash the commits before merging, please?

You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#345 (comment)

Julian K. Arni
Haskell Consultant, Turing Jump
https://turingjump.com

[soenkehahn]

I wouldn't use ... and just match ghc's indentation.

[jkarni]

I feel like that'd be testing indentation, and might break more easily across GHC versions?

[soenkehahn]

I just think it's confusing in the documentation. Other than that, I would run the doctests with one ghc version only. (As discussed before.)

[phadej]

Used Refl to make GHC decide type-equality

[soenkehahn]

So Lax means that we're considering the given servant endpoint as a description of a url? And Strict means that we are talking about real sub-apis, i.e. including request bodies and headers?

If yes, we should name all these things differently, IMO.

[jkarni]

So Lax means that we're considering the given servant endpoint as a description of a url? And Strict means that we are talking about real sub-apis, i.e. including request bodies and headers?

Yes

If yes, we should name all these things differently, IMO.

Agreed. How about URLMatches and EndpointMatches? We'd have to be consistent about usage of endpoint to mean "everything", though.

More generally, what actually is the correct terminology to distinguish between these things?

[phadej]

is @fizruk happy with the current state of this PR?

As I pointed above, name bike-shedding can be done later.

[fizruk]

Is it worth having sections in both code and export list?
Also this one is missing in the export list.

[phadej]

I usually keep some section indicators in the code as well, so it's organised as well. (Sometimes sections in code are in the different order though, thanks to Template Haskell).

Added Logic subsection to the export list.

[fizruk]

Ok, I'm happy with the current state of this PR. 👍