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:
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?
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?
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:
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?
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.
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 fulltest should pass on all of our target platforms.
We manage incompatible changes of features and interfaces in multiple ways:
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.
If the branch necessitates new deprecations, the deprecation list needs sufficient detail to help affected users plan their upgrades.
If you've removed any deprecated items, have you marked them as such?
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:
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
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.
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.
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:
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.
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.
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?