search

haskell-graphql / graphql-api

Write type-safe GraphQL services in Haskell
BSD 3-Clause "New" or "Revised" License
406 stars 35 forks source link

Smart constructor for Name #26

Closed jml closed 7 years ago

jml commented 7 years ago

The quest to remove TODOs never ends. Although this removes one, it makes another.

Builds on https://github.com/jml/graphql-api/pull/24, so review & merge that before reviewing this (or just review https://github.com/jml/graphql-api/pull/26/commits/ddbd8afbb4723f15236a039618a09a925cec0946).

Was going to do QuickCheck instances in this as per #23 but this turned out to be more complicated than expected and also now I really have to go.

Smart constructor for Name

Name is restricted in value, so when we have something that accepts a Name as a parameter, it should be able to trust that it's valid.

Stopped exporting Name constructor and now export two constructors:

  1. makeName
  2. unsafeMakeName

They do what they say on the tin.

There are three use-cases for unsafeMakeName:

  1. In tests, where we are passing a literal.
  2. In Definitions, because we cannot get GHC to enforce a restricted set of values for a Symbol.
  3. In TypeApi, because we're operating on Data.GraphQL.AST directly when we should be operating on a pre-restricted type. Noted as a TODO comment.

I tried adding a HasName type class to Definitions, but couldn't figure it out in the ten minutes I gave myself.

Misc cleanups

teh commented 7 years ago

Looking at commits looks fine but would prefer to see rebase on other branch if you have time.

jml commented 7 years ago

It has been rebased? On Sat, 17 Dec 2016 at 16:28, teh notifications@github.com wrote:

Looking at commits looks fine but would prefer to see rebase on other branch if you have time.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jml/graphql-api/pull/26#issuecomment-267771727, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHq6pjraEaNl3hcj4dDyRtciBsggoziks5rJA2ogaJpZM4LP6m2 .

jml commented 7 years ago

Or I guess approve #24 and I can rebate on master after merging. On Sat, 17 Dec 2016 at 17:58, Jonathan Lange jml@mumak.net wrote:

It has been rebased? On Sat, 17 Dec 2016 at 16:28, teh notifications@github.com wrote:

Looking at commits looks fine but would prefer to see rebase on other branch if you have time.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jml/graphql-api/pull/26#issuecomment-267771727, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHq6pjraEaNl3hcj4dDyRtciBsggoziks5rJA2ogaJpZM4LP6m2 .

jml commented 7 years ago

Sorry about rebase. Had a brain fart. Done now.

Also removed the IsString instance from Name, as it was allowing constructing invalid names. We could re-introduce it and have it panic, which wouldn't be so bad as it would only ever be called on literals. I thought about that and thought that probably it would be less surprising for users if they had to be explicit about it.