Skip to content

Adding Prim.TypeError.QuoteLabel - #3436

Merged
LiamGoodacre merged 6 commits into
purescript:masterfrom
dariooddenino:QuoteLabel
Oct 30, 2018
Merged

Adding Prim.TypeError.QuoteLabel#3436
LiamGoodacre merged 6 commits into
purescript:masterfrom
dariooddenino:QuoteLabel

Conversation

@dariooddenino

@dariooddenino dariooddenino commented Oct 3, 2018

Copy link
Copy Markdown
Contributor

This pull request adds QuoteLabel as described in #3428
The test and doc string are obviously placeholders.

Right now it works, but I replaced prettyPrintString with prettyPrintLabel in matchTypeAtom's match for TypeLevelStrings and I suppose this will break other things.

I don't think I have a way to discriminate if it's working with a label or not. matchTypeAtom takes only TypeRenderOption as argument, but I'm not sure it's appropriate to add a "isLabel" parameter there.

@dariooddenino dariooddenino changed the title tentative code Adding Prim.TypeError.QuoteLabel Oct 3, 2018
Comment thread src/Language/PureScript/Errors.hs Outdated
toTypelevelString (TypeApp (TypeConstructor f) x)
| f == primSubName C.typeError "Quote" = Just (typeAsBox x)
toTypelevelString (TypeApp (TypeConstructor f) x)
| f == primSubName C.typeError "QuoteLabel" = Just (typeAsBox x)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think for this, you don't want to use typeAsBox. You want to match directly on TypelevelString for x, along with Just . line . prettyPrintLabel $ x.

Comment thread src/Language/PureScript/Pretty/Types.hs Outdated
match TypeWildcard{} = Just $ text "_"
match (TypeVar var) = Just $ text $ T.unpack var
match (TypeLevelString s) = Just $ text $ T.unpack $ prettyPrintString s
match (TypeLevelString s) = Just $ text $ T.unpack $ prettyPrintLabel $ Label s

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With the changes to toTypelevelString, you can revert this. This is definitely not desirable 😄

@dariooddenino

Copy link
Copy Markdown
Contributor Author

@natefaubion thanks! I fixed toTypelevelString like you suggested, wrote the docs line and moved the test to its own file.

Comment thread src/Language/PureScript/Docs/Prim.hs Outdated
quoteLabel :: Declaration
quoteLabel = primTypeOf (P.primSubName "TypeError") "QuoteLabel" $ T.unlines
[ "The QuoteLabel type constructor renders any label as a Doc"
, "to be used in a custom type error."

@natefaubion natefaubion Oct 24, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about

The QuoteLabel type constructor renders any Symbol as a syntactically valid label, escaping with quotes as needed.

@LiamGoodacre

Copy link
Copy Markdown
Member

^ I like mentioning the syntactically valid label stuff.
But I'm not sure about the wording of renders because the constructor doesn't actually do anything. The compiler interprets it when it has to satisfy a Warn or Fail constraint. I've had people think Quote/TypeString works like a type family and immediately resolves to a symbol, and it feels to me like this wording suggests that this is the case.

What do you think about?

The QuoteLabel type constructor will produce a Doc when given a Symbol. When the resulting Doc is rendered for a Warn or Fail constraint, a syntactically valid label will be produced, escaping with quotes as needed.

@dariooddenino

Copy link
Copy Markdown
Contributor Author

Is that ok as the definitive version? :)

@natefaubion

Copy link
Copy Markdown
Contributor

I think the only thing is changing the doc text to Liam's suggestion.

Comment thread src/Language/PureScript/Docs/Prim.hs Outdated
quoteLabelDoc :: Declaration
quoteLabelDoc = primTypeOf (P.primSubName "TypeError") "QuoteLabel" $ T.unlines
[ "The `QuoteLabel` type constructor will produce a `Doc` when given a `Symbol`. When the resulting `Doc` is rendered "
, "for a `Warn` or `Fail` constraint, a syntactically valid label will be produces, escaping with quotes as needed."

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Typo: will be produces -> will be produced

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ugh, I even read it 4 times :/ sorry

@LiamGoodacre

Copy link
Copy Markdown
Member

I just noticed that the kindDoc documentation also mentions all the constructors:

kindDoc :: Declaration
kindDoc = primKindOf (P.primSubName "TypeError") "Doc" $ T.unlines
  [ "`Doc` is the kind of type-level documents."
  , ""
  , "This kind is used with the `Fail` and `Warn` type clases."
  , "Build up a `Doc` with `Text`, `Quote`, `Beside`, and `Above`."
  ]

We should add QuoteLabel here too.

Could you also add yourself to the CONTRIBUTORS.md file?

I will happily merge once these two things have been updated.
Thank you for working on this!

@dariooddenino

Copy link
Copy Markdown
Contributor Author

Thank you all for taking the time to review my pr :)
I hope I can keep on contributing (and maybe with more useful stuff! :D)

@LiamGoodacre
LiamGoodacre merged commit c5c965f into purescript:master Oct 30, 2018
@LiamGoodacre

Copy link
Copy Markdown
Member

🎉 Thanks again @dariooddenino !

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.

3 participants