Use stan config files for stan plugin (#3904) #3914
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
michaelpj
reviewed
Dec 18, 2023
There was a problem hiding this comment.
I think this is probably fine, but I do think we should try and get upstream to provide us with a more packaged solution for doing this. Compare with e.g. fourmolu where we just have loadConfig :: FilePath -> Config that gives you the appropriate config given the working directory.
Could you make an issue upstream? or maybe even consider contributing the function?
| let analysis = runAnalysis Map.empty enabledInspections [] [hie] | ||
| let currentHSfromHIEAbs = hie_hs_file hie | ||
| currentHSfromHIERel <- liftIO $ makeRelativeToCurrentDirectory currentHSfromHIEAbs | ||
| -- This codes follows what 'runStan' does, from the module 'Stan' |
There was a problem hiding this comment.
Can you ask upstream to expose this logic from the library so you can reuse it?
| let enabledInspections = HM.fromList [(LSP.fromNormalizedFilePath file, inspectionsIds)] | ||
| -- This should use Cabal config for extensions and Stan config for inspection preferences is the future | ||
| let analysis = runAnalysis Map.empty enabledInspections [] [hie] | ||
| let currentHSfromHIEAbs = hie_hs_file hie |
There was a problem hiding this comment.
you already have the file from the rule argument, I think?
| HM.fromList [(LSP.fromNormalizedFilePath file, inspectionsIds)], | ||
| []) | ||
| ResultL warnings stanConfig -> do | ||
| -- I'm not sure this is the best way to obtain the .cabal |
There was a problem hiding this comment.
Again, ideally stan would expose a function for working out everything it needs given a current directory. We've increasingly found that it's risky to deviate from what a tool does when run at the CLI. For example, by doing this maybe we do a better job of finding the cabal file than the stan code would, so the user will see different results in HLS and the stan CLI. Better to get stan to give us a function that just sets everything up given a working directory, and then we can just call that.
There was a problem hiding this comment.
Maybe we could define the function here that we'd like stan to provide, that gives them something to look at!
| -- This codes follows what 'runStan' does, from the module 'Stan' | ||
|
|
||
| -- There aren't any cli args. isLoud=False=Silent output | ||
| let isLoud = False -- Should this be enabled when debugging? Enables default stan cli output |
There was a problem hiding this comment.
what happens if it's enabled? Does it write to stderr or what? we don't want that!
| -- I'm not sure this is the best way to obtain the .cabal | ||
| -- for this file but it'll have to do. Anyways, if it is not | ||
| -- found it's not a big issue. That was the default previously. | ||
| maybeCabalFileDir <- let maybeCabalFileDir = findFileUpwardsF |
There was a problem hiding this comment.
Also I don't know if you looked around but I bet we do this somewhere else in HLS also...
fendor
requested changes
Dec 19, 2023
|
Thanks for the comments. I'll work on it and see what changes can be made in stan to make this work better. |
This was referenced Dec 21, 2023
|
I've made a PR to stan exposing a bit of the config code, so there is more shared code. However, the analysis section needs to be kept different without some bigger changes to how I've updated the logging constructors. As for the legibility of their results, it's not the best, but at least for me, they have been useful while debugging this, this is how they look: |
|
While they are debug logs, they are currently difficult to read and a bit overly verbose. Having legible logs is important for debugging HLS. Rather than logging the show instance, log the information we care about. For example, in |
|
With some changes, how does this look like? Should I still disable the logging of the checks (even with a .stan.toml file, stan could ignore that via the env var)? |
|
@0rphee Much better! I still think logging the stan hints is overkill, as this will be spammed on every file save, right? |
|
I think the point of DEBUG is to log information that's useful for debugging, which might be a lot. If this is useful for debugging the stan plugin, then I don't see why it shouldn't get logged... |
|
I think this is fine now :) Regarding showing each inspection name, I now agree with @fendor, since it was useful for me while doing this (there were mismatches between the loaded config, and what was actually applied, and that was the only way to notice), but now that same information is (correctly) provided by the first section. This is how now all the debug ouput looks like: |
soulomoon
added a commit
to soulomoon/haskell-language-server
that referenced
this pull request
Jan 12, 2024
commit 2fe2d70 Merge: 034b33e bea1fed Author: fendor <[email protected]> Date: Thu Jan 11 16:05:34 2024 +0100 Merge pull request haskell#3941 from fendor/enhance/cabal-no-diags-if-disabled Don't produce diagnostics if plugin is turned off commit bea1fed Merge: e9aab3c 034b33e Author: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Date: Thu Jan 11 13:54:12 2024 +0000 Merge branch 'master' into enhance/cabal-no-diags-if-disabled commit e9aab3c Author: Fendor <[email protected]> Date: Wed Jan 10 17:18:39 2024 +0100 Don't produce diagnostics if plugin is turned off commit 034b33e Author: 0rphee <[email protected]> Date: Thu Jan 11 02:53:11 2024 -0600 Use stan config files for stan plugin (haskell#3904) (haskell#3914) * Bump stan Needed in order to get the functions for getting the config, etc. * Use stan config files for stan plugin (haskell#3904) * Add test case for .stan.toml configuration * Fix windows tests See kowainik/stan#531 --------- Co-authored-by: Michael Peyton Jones <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Solves #3904
I didn't have a clear idea of how it would be best to get the .cabal file for the current file, but the way I did it seems to work fine, it is not critical for stan anyways. Please tell me if there's a better way, and whether the new export of the function from
ghcide/session-loader/Development/IDE/Session/Implicit.hsis ok.I also added a test case, in my machine it takes around 5-6s.
I'm not aware if it is possible to load the config just one time for each file, but it should be more appropriate than loading it with every change to the file.