Skip to content

Provide externs to rebuildModule #224

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 1 commit into from
May 28, 2021
Merged

Provide externs to rebuildModule #224

merged 1 commit into from
May 28, 2021

Conversation

thomashoneyman
Copy link
Member

@thomashoneyman thomashoneyman commented May 28, 2021

Closes #222. When implementing #209 I accidentally provided an empty array to rebuildModule for the externs; this fix properly threads the externs through. As @JordanMartinez noticed in #222 we also can remove the initNamesEnv argument as it is no longer used.

Evidence that things work once again:

Screen Shot 2021-05-28 at 1 46 10 PM

Copy link
Collaborator

@hdgarrood hdgarrood left a comment

Choose a reason for hiding this comment

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

This is definitely a step in the right direction, but I’m curious about a) whether initEnv is used, and b) if we can get unused bindings to emit errors which cause CI to fail. Ideally this would have been caught before it was merged in the first place.

@JordanMartinez
Copy link
Contributor

whether initEnv is used

It's used in the /search path for doing searches

@JordanMartinez JordanMartinez merged commit bd29070 into master May 28, 2021
@JordanMartinez JordanMartinez deleted the fix-server branch May 28, 2021 23:10
@JordanMartinez
Copy link
Contributor

if we can get unused bindings to emit errors which cause CI to fail

This sounds like a separate PR to me. Perhaps we should follow the hlint warnings that rhendric added to the purescript repo recently?

@hdgarrood
Copy link
Collaborator

No need to bring hlint in for that - GHC is already capable of generating warnings for unused bindings, and then -Werror / —pedantic will cause the build to fail if there are warnings.

@hdgarrood
Copy link
Collaborator

By the way, we could also keep the initNamesEnv around and use P.rebuildModule' here, as that one allows you to provide a prebuilt Env rather than having to construct it each time, as that would speed the server up a little bit. See https://github.com/purescript/purescript/blob/master/src/Language/PureScript/Make.hs, specifically how rebuildModule is the same as the primed version except that it constructs an Env out of the externs first.

@JordanMartinez
Copy link
Contributor

By the way, we could also keep the initNamesEnv around and use P.rebuildModule' here

I would but rebuildModule' is not exported. Not sure if that's intentional or not. That was my initial thought when I saw how similar that was to rebuildModule.

@hdgarrood
Copy link
Collaborator

Ah, I see, thanks. I think that might be unintentional. It would definitely be useful in cases like this one when you’re using the compiler from Haskell.

@JordanMartinez
Copy link
Contributor

I've submitted a PR to export it.

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.

Unable to use modules from package set
3 participants