Comments (17)
Unfortunately I have opinions: While the following hints are lawful transformations, they require reading the codebase with too much context:
Eta reduce
Avoid lambda
Avoid lambda using `infix`
Use tuple-section
Sometimes it is obviously better to lay out things explicitly instead of implicitly, especially if we want to keep the overhead required for examining the codebase to a minimum. I don't mind requiring some domain knowledge about Cabal to read the code, but I particularly hate having to rewrite stuff in my head to expand the tacit forms. It is an unnecessary exercise when there are more important things to focus on.
Edit: @dixonary has stumbled upon the "Eta reduce" lint introducing a compilation error due to the incomplete implementation of Quick Look. This and "Redundant $" sometimes trigger such errors.
from cabal.
I quickly tried some of the other hints (but held back from turning them into pull requests). HLint is such a great tool.
from cabal.
These are perfectly good PRs that improve the codebase. I'm grateful for them. But do we encourage more? This is really tricky. Such code improvements are, relatively, shallow, they don't improve code style consistency in a reliable way (no hlint in CI and others may even undo the changes while working on the same code; it's their right in the absence of CI checks), they cost review time and make wading through patches while tracking a code change over time harder. I'm sure I missed some pros and cons.
Definitely, a PR that removes dead code (after asking around if it's not used in a weird way in some test script) is 10 times more valuable (and there are tools for that, e.g, weeder). The same about deep cabal-specific simplification of the design/algorithm/API//code.
Cabal is a big, complex system, both code and the dev activity, so I'd personally default to "if it ain't broke, don't fix it" or in other words, don't touch unless you have a very good reason (e.g,. a particular hlint hint really seems to you to clarify a code fragment a lot [edit: to the point that, e.g., you volunteer to rebase other people PR's to the end of time if conflicts arise --- in other words, you are willing to put your skin in the game]).
from cabal.
no hlint in CI
Only there is now :-) https://github.com/haskell/cabal/blob/c1659a3f43d57a3e15a32e248b444cbfd069ded0/.github/workflows/lint.yml
PR that removes dead code (after asking around if it's not used in a weird way in some test script) is 10 times more valuable (and there are tools for that, e.g, weeder).
We should open a ticket about Weeder (or find an old one). It looks like the repo still has remnants of a past Weeder setup even:
- https://github.com/haskell/cabal/blob/c1659a3f43d57a3e15a32e248b444cbfd069ded0/weeder.dhall
- https://github.com/haskell/cabal/blob/c1659a3f43d57a3e15a32e248b444cbfd069ded0/cabal.project.weeder
from cabal.
/cc @philderbeast @Mikolaj @gbaz @Kleidukos @ffaf1 @fgaz @andreabedini
from cabal.
I don't have strong opinions on any of those. I have been silently applying some of them in the code I touch, avoiding perhaps eta reductions just to not be too conspicuous :-)
from cabal.
I dislike these
hlint -i "Use infix" -i "Use String" -i "Eta reduce" \
-i "Redundant ==" -i "Use fromMaybe" -i "Use &&&" \
-i "Redundant if" -i "Use isNothing" -i "Use <$>" \
-i "Use $>" -i "Use >=>" -i "Use void" \
-i "Avoid reverse" src/
but in general, for the sake of consistency, if the project decided to turn some warnings on, we act on those.
from cabal.
The effort to make a pull request for a single hlint suggestion can be pretty low (depending on the count and whether hlint --refactor
can be used). If reviewers don't like a change that can be a good time to move a suggestion into a separate section of hints we don't like for this project.
from cabal.
@philderbeast I'm not a big fan of slicing it thin for a reason: it takes energy to process every PR on the reviewers' side, and we have a very limited pool of reviewers. I'd much rather we agree on which hlints we don't like and have one (or couple) PRs fixing the rest, or, at least, what is possible to fix automatically. But I can see value in a slower approach too. Just as one of the people who patrols PRs regularly I see a lot of extra work coming my way with the smaller PRs :-)
from cabal.
I am cautious and prefer the one pull request per hint approach (or if not that then at least one hint per commit). I find it much easier to manage (making the change and reviewing). Something to be wary of, sometimes hlint suggestions cascade. Another thing seen in #9112, there's going to be a bit of tension between hlint and fourmolu wherein a small hlint change leads to a larger fourmolu change.
from cabal.
I like the help and discipline hlint provides, especially to newbies, but I'd at least wait a long while after the fourmolu transition is complete, we have feedback and can reflect on the ordeal and the dust generally settles.
from cabal.
@Mikolaj fair enough. I'm not very keen on "dust settles" timeouts because it's hard to measure. But anyway. Do you want to shut down #9111, #9112?
from cabal.
no hlint in CI
Only there is now :-) https://github.com/haskell/cabal/blob/c1659a3f43d57a3e15a32e248b444cbfd069ded0/.github/workflows/lint.yml
Heh, not a binding one. Though it's one click (in Settings) away, I've just checked.
We should open a ticket about Weeder (or find an old one). It looks like the repo still has remnants of a past Weeder setup even:
Oh, cool. Yes, I think this would also not be controversial (except the initial deletion of any huge blobs reserved for whatever future lofty design).
from cabal.
they cost review time
Yes, this is my main objection to the whole enterprise, and I voiced it above. I'd prefer less PRs that batch several hlints if not one gigantic one to rip it out at once (just like what we did with reformatting).
make wading through patches while tracking a code change over time harder
Technically true, but it doesn't touch too many lines, so I doubt it will be very noticeable. Again, fewer commits (in fewer PRs) would be better: we could add big changes to .git-blame-ignore-revs
at least.
from cabal.
We might get dead code detection with weeder #9121 but I found dead code with hlint.
Do I hold back (until the restraining order is lifted) or do you want to see it (as a pull request)?
from cabal.
Iād be curious to see it but Iād first ask more cautious of us, so, maybe @Mikolaj?..
from cabal.
If it's dead code, it doesn't matter how it got detected. Great find, if so.
from cabal.
Related Issues (20)
- Weird solver behaviour: doesn't consider installed version first HOT 4
- Gotchas of `[TARGETS]`; implicit vs `all` vs combinations HOT 2
- Build failure with GHC-8.2 HOT 16
- How to alert users of known issues with released `cabal-install`? HOT 5
- Documentation for -fdeveloper HOT 3
- Assertion failure with `.tar.gz` file as package. HOT 3
- Incomprehensible "imported by" noise when package doesn't exist HOT 3
- `runhaskell Setup.hs configure --enable-split-sections` etc not documented online
- cabal says "does not match server, will redownload: file length mismatch" but does not redownload
- Disambiguate docs, `cabal` versus Cabal HOT 2
- Broken packages not detected by `ghc-pkg check` using Setup.hs (Gentoo Linux) HOT 2
- Unifying the various documentation flags under the `documentation:` stanza HOT 3
- Windows, foreign-library, Cabal-3.10.3.0 regression in linker HOT 4
- Fake package leaking when no `[TARGETS]`
- Make Setup a separate component
- Add support for logging handles in the Cabal library
- Hide cbits symbols via visibility attribute or compiler flag? HOT 1
- [Ubuntu] [3.12.0.0-prerelease] Semaphore does not exist HOT 16
- Split off `cabal get` command HOT 4
- Restructure finalCheckPackage, moving it earlier in the cabal-install pipeline
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
š Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. ššš
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ā¤ļø Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from cabal.