-
Notifications
You must be signed in to change notification settings - Fork 14
Style Guide
This document outlines some general guidelines for maintaining consistency in coding style, introduction of breaking changes, documentation, and overall design within aqp. We should adhere to these suggestions to the extent that the they are helpful; none of us have the time to constantly nit-pick each other's work.
As we move forward, we should all strive to keep the main branch reserved for working, documented, and well-tested code.
Please be considerate when making suggestions for major changes to other's code, put major change suggestions into an alternate branch, and propose via pull request.
Dependencies should be kept to a minimum as we move forward, with the gradual replacement of plyr and reshape functionality replaced with data.table.
See #140 for active discussion.
Commits:
-
Should pass all expected tests -
devtools::test(). If tests do not pass, explanation for change in test behavior. -
Build and check documentation -
devtools::check_man(). Only need to commit related changes. -
R CMD CHECK passes w/ run-donttest examples -
devtools::check(run_dont_test = TRUE)
The master branch is regularly downloaded by our users, and contributors need it as a stable "base" for their work. Do yourself and others the favor of taking time to review your contributions in a variety of ways. Simply reviewing your own code in a different format may be enough to reveal something you didn't catch.
Folks are encouraged to make a branch (if they are part of the aqp-package-development group), or fork the package and make a branch and submit a pull request (PR) even for small changes.
GitHub Actions are set up to run checks for any PR against master, so it is a great way to check, revise and even show off your work.
The master branch /R folder is not the place to store "experimental" code, or code that has not been discussed as to how it will be integrated into the package. You CAN put it in /misc if it is reasonably polished such that someone else could follow it.
The main feature of GitHub for managing our version control repositories is a rich API for collaboration and code comparison/review. We use a variety of ways of sharing information and code and like to leave options open and flexible for contributors.
At this time it is not "required" to use a PR to commit to the master (production) branch. Nor is there a hard requirement that R CMD check or other Actions pass.
Bug "hot" fixes, limited changes that do not dramatically affect or relate to existing functionality (i.e. all tests passing for a new argument to existing function), and commits to demos in the /misc folder are all acceptable things for direct-to-master. Care should be taken with e.g. function argument naming, and changes to code contributed or recently altered by others.
A PR is strongly encouraged where any coordination with existing functionality is warranted. Discussion and concise ways of reviewing code become necessary in this case, and it is too challenging to track if things are getting mixed into the production branch. All individuals who have permission to commit to the production branch should strongly consider contacting other contributors to the package before adding anything new.
Kent Beck gave four design rules that many programmers have accepted as valuable. An interpretation in parentheses. These are good things to consider when adding a new function to the NAMESPACE or considering adding/exposing additional features.
- Passes the tests (write tests and then write function to specifications)
- Reveals intention (clear, expressive, consistent)
- No duplication (D.R.Y -- don't repeat yourself)
- Fewest elements (be concise)
Martin Fowler summarizes them well here: https://martinfowler.com/bliki/BeckDesignRules.html. The rules are in priority order, so "passes the tests" takes priority over "reveals intention"
Aspirations:
- optimize for intuition ("estimate a thing") and accurate code-completion (
estimate...) vs. pure style adherance - relatively "uniform" syntax that reflects a cohesive, well-thought out package
- minimal number of keystrokes, while still providing essential context as to what a function does
- function names should include verbs -- they "do" something
- arguments (names) that refer to the same data for similar functions should do similar things
- some overlap of functionality is inevitable, but should be minimized and well justified
Use Roxygen for all functions and datasets. We currently need to manually add to NAMESPACE, as Roxygen is not present to generate it (yet). Please strongly consider adding @importFrom where appropriate! Try to use the most recent {roxygen2} package from CRAN and keep the version the package is using up to date.
Now you can use Markdown syntax in Roxygen which makes adding code and formatting a breeze.
Code committed to master should be passing all minimal R CMD CHECK documentation checks. You should run devtools::check_man() regularly to build the latest .Rd files and commit them along with your pull request or commits to the master branch. A good practice is to update the documentation after you have it working the way you want, and commit all affected .R/.Rds as a single related commit.
Do not make other people build your documentation for you if you can avoid it--but it does happen. If you do encounter an un-built documentation file not related to your work you are not obligated to commit it under any circumstance, but you can.
It is not required that Roxygen tags be explicitly specified where they can be inferred e.g. @title @description @details etc.; but it should be easy for a human reader to discern what is what.
You can specify anything explicitly that you want, but in general it is better not to over-specify things like @usage as much of the .Rd can be generated from the function definition adjacent to the Roxygen block. Roxygen blocks should be immediately adjacent to the object they document in the code.
Make appropriate use of white-space and new lines in Roxygen. Use Ctrl+Shift+/ to re-flow code to your RStudio set margin (Global Options >> Code >> Display >> Margin Column).
There is not currently a standard width specified for the package. Some individuals prefer to put paragraphs all on one line and rely on soft-wrapping. But most code bases using this type of in-source documentation use a fixed-width format.
A Margin Column value of 130 or so is probably reasonable balance between number of lines and readability, but not as a hard requirement -- with some manual correction for long URLs. and unusual formatting needs.
Package, functions, citations, documentation.