-
-
Notifications
You must be signed in to change notification settings - Fork 370
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
Apply some hlint suggestions, silence some others. #1227
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ library | |
build-depends: aeson | ||
, base >=4.12 && <5 | ||
, containers | ||
, extra | ||
, foldl | ||
, lsp | ||
, hls-plugin-api | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively: don't incur a dependency just to satisfy hlint's obsession with eitherM. But the rest of hls already depends on extra so why not? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. HLint only suggests extra functions if you pass the appropriate option, so if you don't want to use more option, then don't pass that. |
||
|
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.
Maybe this change has broken the install script??? It shouldn't but is the unique direct change in that component.
@jhrcek please, could you try the install reverting this change manually?
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.
If the change broke it, hlint suggestion was very wrong 🤔
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.
@jneira Just tried with latest master and I had to revert both this change (nubOrdBy back to nubBy) AND one more change done in
install/src/Print.hs
below. After reverting those two changes it started working again.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 don't know if there's some kind of implicit import going on or something ,but those functions come from
extra
and I don't see them imported in either of the 2 modules.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.
No implicit imports afaik, fortunately 🙂
The fix would be add those imports probably
@jhrcek Thanks for reporting it, would you like to open a pr adding the appropiate imports? i'll do myself in other case
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.
Sure, I'll do it later today.
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 guess this part of the code isn't tested by the CI then? Sorry about that.
@jneira thanks for the merge!
@jhrcek thanks for fixing it!
(I did not except this PR to be so fiddly and hard to merge.)
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.
yeah, the install script is not being tested, but it does not change frequently so 🤷