Darcs code can be a little hostile. —Florent
- In an effort to increase new-coder approachability, existing-coder sanity and general maintainability of the codebase, Darcs has adopted Johan Tibell’s style guidelines, found here.
- All submitted patches should adhere to this guide.
- All patches should be checked with hlint before submission.
- Try to avoid cryptic short identifiers (gcau, wspr).
- Any exceptions to the guide will be listed below.
- Long import lists should be wrapped before 80 characters thus:
import Darcs.Foos.Bars ( bars, moreBars, wibbles, wobbles, barWobbles )
That is, wrapped with the first identifier on each new line aligned with the first identifier on the first line.
- Indentation of type-signatures should be one identifier per-line only if you want to comment on each individual parameter:
-- |foo some As and Bs foo :: [A] -> [B] -> [C]
-- |Mmm, per-param comments foo :: [A] -- The As -> [B] -- The Bs -> [C]
-- |foo some As and Bs with bad indentation! foo :: [A] -> [B] -> [C]
Further to the style guidelines we’d really like to add more comments to the codebase, too much of the code has unclear intentions, with no elucidative comments. The general principle is:
- If you’ve written it - Haddock it!
- If you’ve read it and understood it - Haddock it!
- If you’ve read it and can’t understand it - Haddock it, with a question!
- If you’re working in an area of code, ensure it ends up Haddock’d!
It boils down to “What’s the intuition?” for a given function.
- no trailing whitespace
- avoid needless parens (for example when using ++)
- use fmap to simplify returning a result in a monad (e.g
lines `fmap` readFile foo)
- avoid long lines if possible. A rule of thumb is 65 lines (+10, under special circumstances) just as the fmt(1) command suggests
- if your patch fixes issueXYZ, then start the patch name with “resolve isssueXYZ: my comments”
New developers (and old developers alike), state here your griefs against the code, and your proposed solution to them, for the community to turn them into new good habits. Help the Darcs readability revolution!
Please be positive in your comments: no finger-pointing, and as much references to concrete code as possible. Have a nice Catharsis!
- Functions which output a message to the user (such as “do you want to record this patch?”) should have the message in a comment near them, so that they can be found by grepping. Ideally (?) these functions should be in the Darcs.Command.* modules, and Darcs.Internal.* should only have non-interactive functions.
When a module A re-exports a function f from another module B, in a module C which needs f, import it from B rather than A, so as to avoid too many indirections when looking for f.
- Would the exception here be unless we are trying to get some kind of encapsulation going, as with Darcs.Patch and Darcs.Repository?
- Of course, on the other hand, using that exception might also be a hint that there might be some over-encapsulation (aka YAGNI) going on. Its probably something that’s not worth actually chasing in existing code, but it’s a good question to ask oneself before writing new code. Especially module A (the re-exporter) in my example above.
unsafeCoerceP should not be used except in the core. smart_diff is a similarly dangerous function and should be carefully used.
Experiences from grapefruit GraphicalInterface hackers
- currently the terminal-based user interface code of Darcs and the actual application logic are too much intermixed.
These mostly come from a discussion that happened on irc: http://irclog.perlgeek.de/darcs/2010-12-09
- Order of definitions in a file makes a big difference. If definition A depends on B, then we should try to define B near A, not 500 lines away.
- Type witnesses are hard to read for newcomers.
- cleverCommute could be documented so I can read “it makes use of this clever trick”
- First time you read commuteFiledir you ask, what are the cases does it not handle?
- tests that call things identity when they really aren’t
More feedback: http://irclog.perlgeek.de/darcs/2010-12-15#i_3089249
- patch-pair generator for commutation tests only produces 4-7% useful cases
- someone decided that arbitraryTree should produce empty trees 40% of times, but not justified (performance?)