Skip to content

Generate types from the metamodel (attempt 2) #478

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Jun 8, 2023
Merged

Conversation

michaelpj
Copy link
Collaborator

This is a redone version of #458. That foundered on the use of TH, in particular I really struggled to get it to work on multiple versions of GHC. This version is similar, but takes a much more basic approach of just building up big strings for Haskell modules. It works okay; it's much easier to debug; it's more reliable; documentation works better; and it compiles faster. So I think we can go with this.

@michaelpj
Copy link
Collaborator Author

I'm also considering moving to generic-lens and dropping the lens generation, but I'll do that in a follow up PR if so.

@michaelpj
Copy link
Collaborator Author

Of interest to @thomasjm and @joyfulmantis perhaps.

Copy link
Collaborator

@joyfulmantis joyfulmantis left a comment

Choose a reason for hiding this comment

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

Thanks for the hard work Michael! I can't say I'm too familiar with either this code base, or packages that depend on it, but LGTM.

pure $ parens $ "Row.Rec" <+> tyList

genEnum :: Enumeration -> CodeGenM T.Text
genEnum e@Enumeration{name} = do
Copy link
Collaborator

Choose a reason for hiding this comment

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

The gen functions (genStruct, genEnum, genAlias) are basically identical. Would it be better for them to be refactored into one function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They do have a lot in common, but they also have a lot that's different and the input functions are different. I think to share more I'd have to introduce an abstraction for all the pieces of a Haskell datatype, which I think would in itself be quite complicated...

@michaelpj
Copy link
Collaborator Author

Addressed comments, and added NFData instaces.

@thomasjm
Copy link
Collaborator

Possible dumb question--I wanted to build the Haddocks for this but ran into trouble. I added a simple Stack file (see below) and was able to build the branch.

However, when I run stack haddock --open, the generated stuff doesn't seem to be there. The only modules I see are Language.LSP.MetaModel.*. Am I missing something?

# stack.yaml
resolver: lts-19.33
packages:
- lsp-types
extra-deps:
- row-types-1.0.1.2

@michaelpj
Copy link
Collaborator Author

Hmm, it works for me with cabal haddock. I don't know if stack is doing something funny here? You shouldn't see the metamodel types at all, those are from a sub-library that isn't the main one.

Hopefully this makes it either to port hls to the new release
-- | Response error type as defined in the spec.
data ResponseError =
ResponseError
{ _code :: ErrorCodes
Copy link
Collaborator

Choose a reason for hiding this comment

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

ResponseError takes ErrorCodes, but you have two types of error codes: ErrorCodes and LSPErrorCodes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't really know what to do here: the metamodel is weird. They both define some error codes, of which I guess any can be used, but it's not explicitly represented as a type... I guess we could use ErrorCodes |? LSPErrorCodes :/

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we will need to do that, as otherwise, the LSPErrorCodes become useless (and some of them are useful and used in the code)

@michaelpj
Copy link
Collaborator Author

Okay, let's merge this to make iteration easier. It looks like the upgrade in HLS is going okay, so I don't think we'll need to back this out and we can tweak it in master.

@thomasjm
Copy link
Collaborator

I've been converting my projects to lsp-types-2.x and I just wanted to say this new version is really great, very regular and easy to work with. Awesome work @michaelpj !

@michaelpj
Copy link
Collaborator Author

Hooray, I'm glad it's good to use :)

@ChrisPenner
Copy link

ChrisPenner commented Aug 13, 2023

Stoked with the quality of this lib! It's making development of the unison LSP SO much easier 😁

Excited to have access to new LSP features quickly 🙌🏼

Thanks for all your hard work!

return len

contentType = do
_ <- string "Content-Type: "

Choose a reason for hiding this comment

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

This PR accidentally reverted #462 and therefore regressed #461 again: The Content-Type header is no longer parsed.

Can you please reapply the patch from #462?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Argh, not sure how that happened. will fix

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

5 participants