-
-
Notifications
You must be signed in to change notification settings - Fork 409
Delete some dead or deprecated settings #2481
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
Conversation
| , plugins :: !(Map.Map T.Text PluginConfig) | ||
| { checkParents :: CheckParents | ||
| , checkProject :: !Bool | ||
| , hlintOn :: !Bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is hlintOn used? Suspicious
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hlint plugin does use it in its GetHlintDiagnostics rule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this one I foolishly assumed that the comment in the doc was all there was - more purging to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I understand now nevermind...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hlintOn is in the plugin for backwards compat in the deprecation process but that was time ago so it can be deleted
@michaelpj feel free to remove it here or we can do later
|
The build in circleci and stack is failing due to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice to see a +6-87 in a pr, thanks for the xmas clean up! 🎅
f571e1c to
b3ab549
Compare
b3ab549 to
769aa99
Compare
| let hlintOn' = hlintOn config && plcGlobalOn pluginConfig && plcDiagnosticsOn pluginConfig | ||
| ideas <- if hlintOn' then getIdeas file else return (Right []) | ||
| let hlintOn = pluginEnabledConfig plcDiagnosticsOn plugin config | ||
| ideas <- if hlintOn then getIdeas file else return (Right []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is not the deactivation handled automatically and generically by hls-plugin-api?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea how this works 😅 If I do that then the test that changing the config works starts timing out 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm ok i think we should change noHlintDiagnostics for
noHlintDiagnostics :: Assertion
noHlintDiagnostics = expectNoMoreDiagnostics 3 doc "hlint"to avoid the parser hang on some diagnostic which never is emitted (not sure why it works keeping the if but 🤷 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind if I leave that as a future improvement, and leave this PR as just getting rid of the old settings?
|
I think I've purged |
Perhaps controversially, I also removed the disabled tests for diagnostics-on-save and Liquid Haskell. They're in the history if someone wants to resurrect those features.
I'm not very happy with the section in the docs about this: ideally it would display some sensible auto-generated table of settings or something...