Skip to content
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

Stan integration #258 #2908

Merged
merged 15 commits into from
Jul 27, 2022
Merged

Stan integration #258 #2908

merged 15 commits into from
Jul 27, 2022

Conversation

uhbif19
Copy link
Collaborator

@uhbif19 uhbif19 commented May 16, 2022

@uhbif19 uhbif19 force-pushed the stan-support branch 2 times, most recently from 57fa2d1 to 45df923 Compare May 16, 2022 05:27
@uhbif19 uhbif19 changed the title WIP: Stan integration #258 Stan integration #258 May 16, 2022
CODEOWNERS Show resolved Hide resolved
plugins/hls-stan-plugin/src/Ide/Plugin/Stan.hs Outdated Show resolved Hide resolved
plugins/hls-stan-plugin/src/Ide/Plugin/Stan.hs Outdated Show resolved Hide resolved
plugins/hls-stan-plugin/src/Ide/Plugin/Stan.hs Outdated Show resolved Hide resolved
]
++ map (" - " <>) (inspectionSolution inspection)

getHieFile :: NormalizedFilePath -> Action (Maybe HieFile)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like something we should provide alongside the other HIE stuff? WDYT @wz1000 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

At least this needs some comments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@michaelpj For me this function signature looks pretty self-explanatory. What else could be said about it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here are a list of questions that I have about this function after reading it:

  • Why isn't this function provided with the other HIE file functions we have? Is it doing something suspicious? Is there a reason we don't want to provide HIE files? Is it working against the direction of travel with hiedb?
  • How does it work? Are we sure we're constructing HIE files correctly? Can the information we get from the rules be inconsistent with what's coming from hiedb?

I'm sure you thought about a bunch of these things when you were writing it, but all that knowledge is going to be lost unless it gets written down! :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function was written by @pepeiborra , so I do not know the answers #258 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe that we have code to create HieFile in Core.Rules and/or Core.Compile

It would be nice to merge it with getHieFile in order to ensure that it stays working.

Copy link
Collaborator Author

@uhbif19 uhbif19 Jul 20, 2022

Choose a reason for hiding this comment

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

You mean creating separate rule for HieFile?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, just abstracting and reusing the same code everywhere

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pepeiborra I do not know which code should be abstracted.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough

@uhbif19
Copy link
Collaborator Author

uhbif19 commented May 16, 2022

I do not understand why CI fails. Plugin is disabled for GHC < 9, but Cabal build still fails: https://github.com/haskell/haskell-language-server/pull/2908/files#diff-aaf5dad5ddf1d2c16a2e8034c6435719f6eb3327cfe1bcd042df5a84b939afe3R282

@michaelpj
Copy link
Collaborator

@pepeiborra changed how you have a plugin that's only enabled on some versions recently. I'm not sure what the right way is off the top of my head (@pepeiborra if you write something here I'll add it to the contributing docs).

@pepeiborra
Copy link
Collaborator

You need to set the buildable field of your library and possibly the test-suite too.Example

library
if impl(ghc >= 9.2)
buildable: False
else
buildable: True

@uhbif19 uhbif19 force-pushed the stan-support branch 4 times, most recently from 3c812d7 to f8a592a Compare May 17, 2022 21:36
@uhbif19 uhbif19 force-pushed the stan-support branch 2 times, most recently from 49cc3e2 to 3baa6e9 Compare May 18, 2022 15:42
@uhbif19 uhbif19 requested a review from michaelpj May 19, 2022 19:19
@July541
Copy link
Collaborator

July541 commented May 20, 2022

Can we have a document as #2066 mentioned?

@uhbif19 uhbif19 marked this pull request as draft May 20, 2022 10:46
@uhbif19
Copy link
Collaborator Author

uhbif19 commented May 20, 2022

Marked as draft, to squash comits before merging.

@uhbif19
Copy link
Collaborator Author

uhbif19 commented May 21, 2022

I tried to use plugin, but it does not show anything to me.

Does something in VS Code Haskell need to be tweaked for this to work?
https://github.com/haskell/vscode-haskell/blob/e81160dbdb6be0ac8493f6dbc0983d27fa096ac7/package.json#L346

@pepeiborra
Copy link
Collaborator

Does something in VS Code Haskell need to be tweaked for this to work?

Not at all. I am in my phone right now, but if the test suite passes you should see the diagnostics in VSCode already.

@michaelpj
Copy link
Collaborator

I'm not sure that's true. I think all plugins automatically get a "globalOn" option that needs to be set to enable them. I'm not sure if there's a way to turn it on by default when testing (@berberman ?).

For vscode, we have an ongoing issue that nobody seems to be sure what the deal is with options in the vscode extension (see #2827). It would be great if someone could find out and help us document it.

@July541
Copy link
Collaborator

July541 commented May 22, 2022

if the test suite passes you should see the diagnostics in VSCode already.

Agree this.

Noticed you are using hint severity,it may not showy as you expected in vscode.

@berberman
Copy link
Collaborator

I'm not sure that's true. I think all plugins automatically get a "globalOn" option that needs to be set to enable them. I'm not sure if there's a way to turn it on by default when testing (@berberman ?).

If you don't specify globalOn in the lsp config for a plugin, which can be set by sendNotification SWorkspaceDidChangeConfiguration DidChangeConfigurationParams ... during the test, the default value True will be used:

instance Default PluginConfig where
def = PluginConfig
{ plcGlobalOn = True

So no worries about globalOn.

For vscode, we have an ongoing issue that nobody seems to be sure what the deal is with options in the vscode extension (see #2827). It would be great if someone could find out and help us document it.

Yes, I can write a documentation for that later.

@pepeiborra
Copy link
Collaborator

is this still in progress or ready for review?

@uhbif19 uhbif19 marked this pull request as ready for review July 15, 2022 14:56
@uhbif19
Copy link
Collaborator Author

uhbif19 commented Jul 15, 2022

@pepeiborra Marked as ready to review.

@pepeiborra
Copy link
Collaborator

@uhbif19 there are stack build failures

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

Looks good to me, but it would be nice to make the suggested refactoring

.github/workflows/test.yml Show resolved Hide resolved
]
++ map (" - " <>) (inspectionSolution inspection)

getHieFile :: NormalizedFilePath -> Action (Maybe HieFile)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, perhaps move this function to the ghcide package and refactor other pieces of similar code around the Core/Compile.hs and Core/Rules.hs modules.

Did you have a chance to look into this refactoring?

@pepeiborra pepeiborra enabled auto-merge (squash) July 22, 2022 22:44
@uhbif19 uhbif19 requested a review from July541 July 23, 2022 21:11
@uhbif19
Copy link
Collaborator Author

uhbif19 commented Jul 27, 2022

@michaelpj CircleCI stuck again.

@pepeiborra pepeiborra merged commit 078fc14 into haskell:master Jul 27, 2022
sloorush pushed a commit to sloorush/haskell-language-server that referenced this pull request Sep 12, 2022
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.

6 participants