docs/project/merge_review_guidelines.pod - Guidelines for pending merge reviews
To maintain Parrot's standards of quality, we evaluate each branch proposed for merging to trunk in terms of several criteria. Not every criterion applies to every branch; these guidelines are guidelines to which we apply our best judgment. As well, these guidelines are not exhaustive.
The purpose of the branch governs the amount and type of documentation it requires. Documentation falls into three broad categories:
User-visible documentation
How do users (language developers, people running Parrot directly, people embedding Parrot, people writing PIR, people packaging Parrot) use the feature? What do they need to know to enable it, how does it work, and what configuration options are available?
Design documentation
How does the feature fit into Parrot as a whole? What design considerations did you make? Are there patterns you followed, or is there literature to read?
Developer documentation
What functions are available, and to whom are they available? What data structures are present, and what do other developers need to understand about them? What are areas of future work, and what are invariants that underlie the whole system?
We know that well-tested features work and we know that well-tested features will continue to work. Under-tested features give us and users much less confidence. A well-tested branch demonstrates several attributes:
Coverage
A full coverage report from an automated testing tool is very valuable, but in lieu of that the tests for the branch's feature should pass the eyeball test. Is everything documented as working tested effectively? If there are gaps in testing, are they clear? Do they have tickets for cage cleaners and other volunteers?
Language Testing
If your feature affects languages running on Parrot (and what feature doesn't?), the branch needs testing from a couple of major languages to demonstrate that it does not harm those languages. If those languages need changes to accommodate the branch, we must work with the language to schedule those changes or to review them as per our deprecation policy.
Platform Testing
Does your feature work on the platform combinations we support? Be especially aware of the differences between C and C++ compilers and 32-bit and 64-bit support, as well as any deviations from POSIX.
make fulltestshould pass on all of our target platforms.
We manage incompatible changes of features and interfaces in multiple ways:
Replacements for removed features
If the branch supplants or supersedes an existing feature, follow the deprecation policy to provide alternatives, shims, compatibility layers, and whatever other mechanisms the deprecation notice promises.
New deprecations recorded
If the branch necessitates new deprecations, the deprecation list needs sufficient detail to help affected users plan their upgrades.
Removals marked clearly
If you've removed any deprecated items, have you marked them as such?
User-visible exclusions to policy marked and dated clearly
If you need any exclusions to the deprecation policy, have you asked for and received them? Have you documented them appropriately?
Branches may implement features requested on the roadmap in whole or in part. They may also affect the schedule of other roadmap items. Have you documented the implications?
Any branch proposed for merging must meet our quality standards in several areas not previously mentioned:
Coding standards
At a minimum, the code must pass all of our active coding standards tests. It must also follow our naming conventions and organizational principles. This means review from other developers. This also means a clean run of
make codingstd.User-visible features
We have no strict guideline for how user-visible features should work apart from a few systems (vtables, embedding and extension API). In general, any public features need review from the user point of view.
API review
Internal features for developers also need a review, especially of any functions or data structures you expose to other parts of Parrot. Where possible, stick with Parrot conventions, especially for constness and the avoiding of null parameters.
Performance characteristics
How does your branch affect performance on our selected benchmarks? For hosted languages? Does it have memory leaks? Does it affect memory use or startup time? We have traditionally let these questions go unanswered, but we can be more disciplined here.
Your branch must also integrate into Parrot as well as possible. In particular:
Manage dependencies
This includes proper dependencies in our configuration and build system for building the code as well as any external configuration or dependencies. Certain parts of Parrot core can depend on external tools such as NQP, while others cannot.
Identify configuration options
If your feature adds configuration options, they need documentation and review outside of the code itself. If your feature depends on configuration options, it needs explicit testing and documentation. This should be self-evident, but it is worth detailed review.
Encapsulation and isolation
Does your branch respect the encapsulation of other parts of Parrot? Does it provide its own sensible encapsulation boundaries? If you need to make changes to other parts of Parrot, should we consider them as a separate branch?