Skip to content

Comments

Remove uses of servant-generic in public Galley API#2022

Merged
pcapriotti merged 1 commit intodevelopfrom
pcapriotti/no-servant-generics
Jan 11, 2022
Merged

Remove uses of servant-generic in public Galley API#2022
pcapriotti merged 1 commit intodevelopfrom
pcapriotti/no-servant-generics

Conversation

@pcapriotti
Copy link
Contributor

The generic mechanism in Servant for defining API types is cumbersome and slow to compile. This commit replaces the API record for the public Galley API with a tree explicitly built using the :<|> operator.

To make up for the lack of record labels, it also introduces a Named combinator for servant API types, which can be used to tag an endpoint with a name (usually a symbol). The name must match the one with which the handler is wrapped.

Compile times are indeed better with this change, but unfortunately not by much.

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • changelog.d contains the following bits of information (details):
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

@pcapriotti pcapriotti force-pushed the pcapriotti/no-servant-generics branch from 41e7234 to 4c29473 Compare January 4, 2022 10:23
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections from me, just a few questions. Let's see what other people think! Maybe wait for another approval or two?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the more tree-shaped structure! Much nicer than the comments over sections of a huge record.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasn't the named record fields thing coincidental, and the true purpose of using records was to have better error messages?

what do the error messages look like with named routes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Named stuff compensates for the fact that handlers have to be matched up with their types in order. If you swap two similar unnamed handlers by mistake, you won't get an error if their types are the same. By using Named types and handlers, we get a name mismatch error (which actually looks reasonably nice). I don't mind getting rid of the Named trick altogether, if people think it's not necessary.

Errors messages are in general horrible. GHC unhelpfully spews out the whole API type multiple times in the output, forcing you to use some kind of pager to be able to see the top of the error message. It wasn't much better before, but I have to admit this change did not improve things in this regard.

For example, if you forget a handler, GHC points you to the the last one, instead of the spot where the handler is missing. If you misplace a handler, it just says that some types don't match, with no indication to where the problem is.

There are probably ways to improve this, but I'm in general a bit sceptical about meddling with error messages, as it can result in very confusing messages sometimes (e.g. in servant's Union stuff).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks for the background. now i think i prefer the 'Named' thing, but only slightly.

no further questions! once we've had a few more eyes i'm happy with these changes.

@pcapriotti pcapriotti force-pushed the pcapriotti/no-servant-generics branch 2 times, most recently from 0a42eb8 to 2155211 Compare January 10, 2022 09:51
The generic mechanism in Servant for defining API types is cumbersome
and slow to compile. This commit replaces the API record for the public
Galley API with a tree explicitly built using the `:<|>` operator.

To make up for the lack of record labels, it also introduces a `Named`
combinator for servant API types, which can be used to tag an endpoint
with a name (usually a symbol). The name must match the one with which
the handler is wrapped.
@pcapriotti pcapriotti force-pushed the pcapriotti/no-servant-generics branch from 2155211 to 04d72de Compare January 10, 2022 16:28
@pcapriotti pcapriotti changed the title Replace servant-generic with Named combinator Remove uses of servant-generic in public Galley API Jan 11, 2022
@pcapriotti pcapriotti merged commit c75de62 into develop Jan 11, 2022
@pcapriotti pcapriotti deleted the pcapriotti/no-servant-generics branch January 11, 2022 07:32
@akshaymankar akshaymankar mentioned this pull request Jan 18, 2022
@pcapriotti pcapriotti mentioned this pull request Feb 3, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants