search

natefaubion / purescript-language-cst-parser

PureScript CST Parser written in PureScript
MIT License
49 stars 17 forks source link

Make instance name and separator optional #18

Closed i-am-the-slime closed 3 years ago

i-am-the-slime commented 3 years ago

Closes #17 and makes :: and the name in instance declarations optional as per PureScript 0.14.2.

I'm not sure if the parser might be too lenient now, because I'm not a 100% sure about what's optional now and in which combination.

JordanMartinez commented 3 years ago

This differs from PureScript only slightly. In PureScript, the name and separator are parsed together. However, your version makes it possible to have a name without a separator or vice versa, which is not what PureScript does. Either both are there, none, or a parsing error occurs.

i-am-the-slime commented 3 years ago

@JordanMartinez I updated it to work like this (I think). Could you have another look? Also my code seems out of place with the rest of the codebase, I wonder if there's a better way to model this.

i-am-the-slime commented 3 years ago

@JordanMartinez Do you think I should turn this into a tuple, too then? Or change the way I do foldMap?

JordanMartinez commented 3 years ago

:man_shrugging: I don't know. I'd ask Nate.

natefaubion commented 3 years ago

Other constructors like this use Maybe (Tuple ... ...) (eg. constraints). Records vs Tuple is obviously contentious, but since Tuple is used elsewhere, I'd use that for now. I'd also go ahead and just shorten it to name rather than nameAndSeparator (again, like constraints).

type InstanceHead e =
  { keyword :: SourceToken
  , name :: Maybe (Tuple (Name Ident) SourceToken)
  , constraints :: Maybe (Tuple (OneOrDelimited (Type e)) SourceToken)
  , className :: QualifiedName Proper
  , types :: Array (Type e)
  }
natefaubion commented 3 years ago

Do you mind adding yourself to Contributors.md?

natefaubion commented 3 years ago

This is fixed in https://github.com/natefaubion/purescript-language-cst-parser/commit/ebefb58d2668caf62c1b8b05fa10afa5a54dc647

i-am-the-slime commented 3 years ago

Ha! Of course I saw the notification about this getting closed, but not the one with your review!

Sorry for not getting back to it, and great that this is now supported!