Comments (5)
Ah, so if we merge #234, hlint will not pester us about any of the suggestions listed there! I had misunderstood the purpose of that list, I thought it was the opposite. Sure, let's merge it!
from klister.
Yes please! We are thankful for the help. Most of these should be extremely minor changes. The only one that will be difficult and that I think you should avoid are the incomplete pattern matches in the Evaluator. Fixing these will mean re-architecting the Evaluator and is just not worth it at this time.
from klister.
As discussed in #234, we do not consider all of hlint's suggestions to be improvements. @doyougnu, @david-christiansen and I met and agreed that the following suggestions are usually good:
[ ] Use $>, 2 hints
[ ] Use <&>, 1 hint
[ ] Use >=>, 1 hint
[ ] Use asks, 5 hints
[ ] Use evalState, 2 hints
[ ] Use fold, 1 hint
[ ] Use for, 1 hint
[ ] Use fromMaybe, 1 hint
[ ] Use id, 6 hints
[ ] Use unless, 3 hints
[ ] Use unwords, 1 hint
[ ] Use void, 5 hints
[ ] Use when, 2 hints
and the following suggestions are usually bad:
[ ] Avoid lambda
[ ] Eta reduce, 12 hints
[ ] Fuse foldr/map, 1 hint
[ ] Move brackets to avoid $, 6 hints
[ ] Redundant $, 32 hints
[ ] Redundant <$>, 17 hints
[ ] Redundant bracket, 12 hints
[ ] Replace case with fromMaybe, 3 hints
[ ] Use tuple-section, 2 hints
[ ] Use uncurry, 2 hints
This leaves the following suggestions which are sometimes good and sometimes bad.
- Use camelCase, 2 hints: some codebases use camelCase for exported identifiers and snake_case for private identifiers. the 2 hints happen to point at a case where we should have used camelCase, so this would probably be a good hint for our codebase.
- Use newtype instead of data, 3 hints:
newtype
might be more space efficient thandata
, but it does not have the same meaning; it is sometimes useful to define adata Thunk a = MkThunk a
wrapper in order to introduce some intentional laziness. in our codebase, however, most of our fields are strict, so there is no semantic difference. another place wheredata
makes more sense thannewtype
is with a record which happens to only have one field right now, but might have more in the future, as opposed to a wrapper type. I seem to have a different version of hlint than you, because my hlint report only finds one hint, not 3, so we only looked atScopedEmpty
, and that one does seem like it would benefit from being turned into a newtype. what are the other 2? const
and record patterns are both case by case, and we did not look at them individually, so I think we should leave them alone.
thanks for your interest in contributing to Klister!
from klister.
Thanks @gelisam putting effort into this upfront, way more than I expected. Sorry if that was a drag on your time.
If it is OK by you, I'd prefer to tackle pull requests for hlint suggestions one suggestion at a time1. To enable that, would it be OK to merge #234 for starters.
For the suggestions you consider bad, we can move them to their own section in .hlint.yaml
like we did in liquid-fixpoint/.hlint.yaml
If you'd like a github action for this, we could do that by adding haskell-actions/hlint actions.
As for tackling a single suggestion, that would involve deleting a single - ignore ...
line in .hlint.yaml
and following the suggestions, sometimes using hlint --refactor
2 and sometimes doing it manually.
Footnotes
-
Keeps the changes small, the reviews lightweight and avoids cascading hlint suggestions where following one suggestion unveils a new suggestion. ↩
-
apply-refact
can easily mess up, such as wiping out CPP conditionals or repeating comments. ↩
from klister.
I seem to have a different version of hlint than you, because my hlint report only finds one hint
Yes that is likely. If we go for an action, the hlint version can be set there.
from klister.
Related Issues (20)
- Set of problems should be open-ended HOT 1
- Comparison with other meta programming approach? HOT 18
- generic programming via type-case
- syntactic marker for binding sites
- Add Bit operations and literals
- section syntax? HOT 5
- explain the limits of the type system HOT 1
- macro from local function
- Slow tests are slow because of poor branch prediction
- Implement Stack Traces
- Klister custom type systems
- 3 failing keyword tests HOT 4
- distinguish between our bugs and the user's bugs
- Let macros yield to other macros with the same name HOT 2
- type-literals for the later phase
- combine Type values HOT 1
- local-expand-type
- local-expand HOT 2
- union errors
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 klister.