Darcs code can be a little hostile -- Florent
How can we fix it?
New habits
- haddock
- type signatures for top level functions! this used to be enforced by -Werror and now it's to be enforced by nagging from the Review Team
- API-style comments (not just relying on the interspersed user-manual for implicit documentation)
- When using explicit lists in code, for example for options, use one entry per line as it makes it easy to write small and clean patches if a list entry gets changed later
- Write tests! Tests are the easiest way to convince others that your code does what you intend it to do.
Send in QuickCheck properties for things you write.
Reforms to old habits
Changes to old code we would welcome
- avoid cryptically short function names (gcau, wspr)
- favour camelCase for top level functions (instead of the current camel/underscore hodgepodge)
- yes, underscores can be useful in certain situations, don't be overzealous
- In the existing code, camelCase is typically used for names that are exported from modules and underscores for names that are internal to a module.
- If you study existing code to understand it, add haddocks summarizing what you learned. This will help others save time in the future.
Minor stylistic details
- 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"
Cahier de doléance
New developers (and old developpers 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 reexports 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-encapsultation (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 reexporter) in my example above.
- Would the exception here be unless we are trying to get some kind of encapsulation going, as with Darcs.Patch and Darcs.Repository?
- unsafeCoerceP should not be used except in the core. smart_diff is a similarly dangerous function and should be carefully used.