Changeset View
Changeset View
Standalone View
Standalone View
src/docs/contributor/contributing_code.diviner
@title Contributing Code | @title Contributing Code | ||||
@group detail | @group detail | ||||
Effective June 1, 2021: Phabricator is no longer actively maintained, and no longer accepting contributions. | Phorge is an open-source project, and welcomes contributions from the community | ||||
at large. However, there are some guidelines we ask you to follow. | |||||
Overview | |||||
======== | |||||
The most important parts of contributing code to Phorge are: | |||||
- File a task with a bug report or feature request //before// you write code. | |||||
- We do not accept GitHub pull requests. | |||||
- Some alternative approaches are available if your change isn't something | |||||
we want to bring upstream. | |||||
The rest of this article describes these points in more detail, and then | |||||
provides guidance on writing and submitting patches. | |||||
If you just want to contribute some code but don't have a specific bug or | |||||
feature in mind, see the bottom of this document for tips on finding ways to get | |||||
started. | |||||
For general information on contributing to Phorge, see | |||||
@{article:Contributor Introduction}. | |||||
Coordinate First | |||||
================ | |||||
Before sending code, you should file a task describing what you'd like to write. | |||||
When you file a task, mention that you'd like to write the code to fix it. We | |||||
can help contextualize your request or bug and guide you through writing an | |||||
upstreamable patch, provided it's something that's upstreamable. If it isn't | |||||
upstreamable, we can let you know what the issues are and help find another | |||||
plan of attack. | |||||
You don't have to file first (for example, if you spot a misspelling it's | |||||
normally fine to just send a diff), but for anything even moderately complex | |||||
you're strongly encouraged to file first and coordinate with the upstream. | |||||
Rejecting Patches | |||||
================= | |||||
If you send us a patch without coordinating it with us first, it will probably | |||||
be immediately rejected, or sit in limbo for a long time and eventually be | |||||
rejected. The reasons we do this vary from patch to patch, but some of the most | |||||
common reasons are: | |||||
**Unjustifiable Costs**: We support code in the upstream forever. Support is | |||||
enormously expensive and takes up a huge amount of our time. The cost to support | |||||
a change over its lifetime is often 10x or 100x or 1000x greater than the cost | |||||
to write the first version of it. Many uncoordinated patches we receive are | |||||
"white elephants", which would cost much more to maintain than the value they | |||||
provide. | |||||
As an author, it may look like you're giving us free work and we're rejecting it | |||||
as too expensive, but this viewpoint doesn't align with the reality of a large | |||||
project which is actively supported by a small, experienced team. Writing code | |||||
is cheap; maintaining it is expensive. | |||||
By coordinating with us first, you can make sure the patch is something we | |||||
consider valuable enough to put long-term support resources behind, and that | |||||
you're building it in a way that we're comfortable taking over. | |||||
**Not a Good Fit**: Many patches aren't good fits for the upstream: they | |||||
implement features we simply don't want. Coordinating with us first helps | |||||
make sure we're on the same page and interested in a feature. | |||||
The most common type of patch along these lines is a patch which adds new | |||||
configuration options. We consider additional configuration options to have | |||||
an exceptionally high lifetime support cost and are very unlikely to accept | |||||
them. Coordinate with us first. | |||||
**Not a Priority**: If you send us a patch against something which isn't a | |||||
priority, we probably won't have time to look at it. We don't give special | |||||
treatment to low-priority issues just because there's code written: we'd still | |||||
be spending time on something lower-priority when we could be spending it on | |||||
something higher-priority instead. | |||||
If you coordinate with us first, you can make sure your patch is in an area | |||||
of the codebase that we can prioritize. | |||||
**Overly Ambitious Patches**: Sometimes we'll get huge patches from new | |||||
contributors. These can have a lot of fundamental problems and require a huge | |||||
amount of our time to review and correct. If you're interested in contributing, | |||||
you'll have more success if you start small and learn as you go. | |||||
We can help you break a large change into smaller pieces and learn how the | |||||
codebase works as you proceed through the implementation, but only if you | |||||
coordinate with us first. | |||||
**Generality**: We often receive several feature requests which ask for similar | |||||
features, and can come up with a general approach which covers all of the use | |||||
cases. If you send us a patch for //your use case only//, the approach may be | |||||
too specific. When a cleaner and more general approach is available, we usually | |||||
prefer to pursue it. | |||||
By coordinating with us first, we can make you aware of similar use cases and | |||||
opportunities to generalize an approach. These changes are often small, but can | |||||
have a big impact on how useful a piece of code is. | |||||
**Infrastructure and Sequencing**: Sometimes patches are written against a piece | |||||
of infrastructure with major planned changes. We don't want to accept these | |||||
because they'll make the infrastructure changes more difficult to implement. | |||||
Coordinate with us first to make sure a change doesn't need to wait on other | |||||
pieces of infrastructure. We can help you identify technical blockers and | |||||
possibly guide you through resolving them if you're interested. | |||||
No Prototype Changes | |||||
==================== | |||||
With rare exceptions, we do not accept patches for prototype applications for | |||||
the same reasons that we don't accept feature requests or bug reports. To learn | |||||
more about prototype applications, see | |||||
@{article:User Guide: Prototype Applications}. | |||||
No Pull Requests | |||||
================ | |||||
We do not accept pull requests on GitHub: | |||||
- Pull requests do not get lint and unit tests run, so issues which are | |||||
normally caught statically can slip by. | |||||
- Phorge is code review software, and developed using its own workflows. | |||||
Pull requests bypass some of these workflows (for example, they will not | |||||
trigger Herald rules to notify interested parties). | |||||
- GitHub is not the authoritative master repository and we maintain a linear | |||||
history, so merging pull requests is cumbersome on our end. | |||||
- If you're comfortable enough with Phorge to contribute to it, you | |||||
should also be comfortable using it to submit changes. | |||||
Instead of sending a pull request, use `arc diff` to create a revision on the | |||||
upstream install. Your change will go through the normal Phorge review | |||||
process. | |||||
(GitHub does not allow repositories to disable pull requests, which is why | |||||
it's technically possible to submit them.) | |||||
Alternatives | |||||
============ | |||||
If you've written code but we're not accepting it into the upstream, some | |||||
alternative approaches include: | |||||
**Maintain a local fork.** This will require some ongoing effort to port your | |||||
changes forward when you update, but is often very reasonable for simple | |||||
changes. | |||||
**Develop as an application.** Many parts of Phorge's infrastructure are | |||||
modular, and modularity is increasing over time. A lot of changes can be built | |||||
as external modules or applications without forking Phorge itself. There | |||||
isn't much documentation for this right now, but you can look at | |||||
how other applications are implemented, and at other third-party code that | |||||
extends Phorge. | |||||
**Rise to prominence.** We're more willing to accept borderline changes from | |||||
community members who are active, make multiple contributions, or have a history | |||||
with the project. This is not carte blanche, but distinguishing yourself can | |||||
make us feel more comfortable about supporting a change which is slightly | |||||
outside of our comfort zone. | |||||
Writing and Submitting Patches | |||||
================== | |||||
To actually submit a patch, run `arc diff` in `phorge/` or `arcanist/`. | |||||
When executed in these directories, `arc` should automatically talk to the | |||||
upstream install. You can add #blessed_reviewers as a reviewer. | |||||
You should read the relevant coding convention documents before you submit a | |||||
change. If you're a new contributor, you don't need to worry about this too | |||||
much. Just try to make your code look similar to the code around it, and we | |||||
can help you through the details during review. | |||||
- @{article:General Coding Standards} (for all languages) | |||||
- @{article:PHP Coding Standards} (for PHP) | |||||
- @{article:Javascript Coding Standards} (for Javascript) | |||||
In general, if you're coordinating with us first, we can usually provide | |||||
guidance on how to implement things. The other articles in this section also | |||||
provide information on how to work in the Phorge codebase. | |||||
Next Steps | |||||
========== | |||||
Continue by: | |||||
- returning to the @{article:Contributor Introduction}. |
Content licensed under Creative Commons Attribution-ShareAlike 4.0 (CC-BY-SA) unless otherwise noted; code licensed under Apache 2.0 or other open source licenses. · CC BY-SA 4.0 · Apache 2.0