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

Log fourmolu and ormolu version that hls using #3744

Merged
merged 7 commits into from
Aug 8, 2023

Conversation

July541
Copy link
Collaborator

@July541 July541 commented Aug 5, 2023

Since fourmolu exported its version from fourmolu-0.12, we can utilize this to log version info.

And this pr also adds the missing fourmolu-0.13 dependency...

@July541 July541 requested a review from georgefst as a code owner August 5, 2023 06:37
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

At the moment this will be fairly noisy: it's logged at Info and it's going to be logged every time there's a formatting request. It seems to me like it would be better to do it once when the plugin is set up, if we can? That might require doing a "version probe" of the CLI tool at startup, but that seems okay?

plugins/hls-fourmolu-plugin/src/Ide/Plugin/Fourmolu.hs Outdated Show resolved Hide resolved
plugins/hls-fourmolu-plugin/src/Ide/Plugin/Fourmolu.hs Outdated Show resolved Hide resolved
@July541
Copy link
Collaborator Author

July541 commented Aug 5, 2023

That might require doing a "version probe" of the CLI tool at startup, but that seems okay?

I think this is vary from #3660, I just want to add some tracing to keep which formatter is using, hence we can locate the problem like haskell/vscode-haskell#920 easily.

How about downgrading this log to debug level?

Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

I'm fine with this. I'd love to do it more systematically, but this is still an improvement!

@July541 July541 changed the title Log fourmolu version that hls using Log fourmolu and ormolu version that hls using Aug 7, 2023
@July541
Copy link
Collaborator Author

July541 commented Aug 7, 2023

Also log ormolu version.

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

Please update the test cases as well!

In particular, since the plugins now use the logger infrastructure, change mkPluginTestDescriptor' to mkPluginTestDescriptor for both plugins.

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM

@fendor fendor enabled auto-merge (rebase) August 7, 2023 18:35
@fendor fendor merged commit 6444903 into haskell:master Aug 8, 2023
42 checks passed
@fendor fendor mentioned this pull request Aug 8, 2023
19 tasks
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.

3 participants