Skip to content

Conversation

@jhrcek
Copy link
Contributor

@jhrcek jhrcek commented Apr 12, 2022

I'd like to contribute some more substantial fixes in the near future, but first please allow me to fix bunch of inconsequential hlint warnings, mostly just removing unused / unnecessary stuff

Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

99.9% of the suggestions are for the better! (For the rest, see my comments.)
You got my blessing! (Assuming these rewrites are correct.)

@andreasabel andreasabel added the re: code quality Concerning the code quality of the implementation of `hackage-server` label Apr 12, 2022
@gbaz
Copy link
Contributor

gbaz commented Apr 12, 2022

Thanks for the thorough review Andreas. @jhrcek feel free to take into account the suggestions or not. When you're ready for a merge, let me know.

@jhrcek
Copy link
Contributor Author

jhrcek commented Apr 12, 2022

Thank you for reviews, I consider this ready to merge.

@gbaz gbaz merged commit 8ec64a1 into haskell:master Apr 12, 2022
@gbaz
Copy link
Contributor

gbaz commented Apr 12, 2022

I should add btw that the hlint changes are pretty minor as such, but since github was running them on every PR, cleaning up that noise really made development nicer. Thanks!

@jhrcek
Copy link
Contributor Author

jhrcek commented Apr 13, 2022

I should add btw that the hlint changes are pretty minor as such, but since github was running them on every PR, cleaning up that noise really made development nicer. Thanks!

Thanks for the feedback. I wasn't really sure if hlint fixes would be welcome by the maintainers, so I only fixed relatively conservative subset of warnings that I thought would be accepted by most people as obvious improvements.
Can you please tell me what exactly are you referring to by " cleaning up that noise"? Is it some view of hlint warnings in PRs? Is it something produced by specific configuration of github actions/travis (if so can you point me to it so I understand where it's coming from)?

Anyway if you agree I will open followup PR, fixing more hlint warnings that make sense to be fixed.
In the end I could add an hlint config to ignore the rest of warnings that we agree we would want to ignore..

@jhrcek jhrcek deleted the jhrcek/hlint branch April 13, 2022 04:14
@andreasabel
Copy link
Member

I should add btw that the hlint changes are pretty minor as such, but since github was running them on every PR, cleaning up that noise really made development nicer. Thanks!

Thanks for the feedback. I wasn't really sure if hlint fixes would be welcome by the maintainers, so I only fixed relatively conservative subset of warnings that I thought would be accepted by most people as obvious improvements.

Yes, I think this is very important. I have some bad experiences with some external contributor pushing some hlint-suggested refactorings into the Agda code base, and some had quite negative effects on code readability. This is why I looked at each and every of your hlint-suggested changes in this PR and checked whether it aids readability.

Can you please tell me what exactly are you referring to by " cleaning up that noise"? Is it some view of hlint warnings in PRs? Is it something produced by specific configuration of github actions/travis (if so can you point me to it so I understand where it's coming from)?

Er, we get a lot of noise from GHC warnings (deprecated imports, incompleted pattern matching) but I do not think we have a hlint workflow afaik, so maybe @gbaz confused these two.

Anyway if you agree I will open followup PR, fixing more hlint warnings that make sense to be fixed. In the end I could add an hlint config to ignore the rest of warnings that we agree we would want to ignore..

That sounds reasonable.
Let me stress again that one should maintain a critical stance towards hlint since not all suggestions help readability. E.g. introducing foldr/l often makes code harder to read. Also, anecdotally, some hlint refactorings are unsound (break parsing or associate operators wrongly).

That said, I am happy to look at another round of likely-not-controversial hlint refactorings.
You did a very good job on this PR, @jhrcek !

@gbaz
Copy link
Contributor

gbaz commented Apr 15, 2022

right -- i confused the two because there are other codebases that do toss hlint warnings at me. whoops :-)

It would be nice to fix the ghc warnings for pattern matching as well I suppose, although doing it "right" would certainly take a bit of thought.

@jhrcek
Copy link
Contributor Author

jhrcek commented Apr 15, 2022

It would be nice to fix the ghc warnings for pattern matching as well I suppose, although doing it "right" would certainly take a bit of thought.

Those should be fairly easy to work around. I will look at fixing those tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

re: code quality Concerning the code quality of the implementation of `hackage-server`

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants