-
-
Notifications
You must be signed in to change notification settings - Fork 378
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Delete some dead or deprecated settings #2481
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...