From 6c498e208e2cc2843b77792c3649331e8507b6c3 Mon Sep 17 00:00:00 2001 From: Brandon Barker Date: Thu, 26 Sep 2024 19:56:19 -0400 Subject: [PATCH] work on contributing text --- doc/sphinx/src/contributing.rst | 189 +++++++++++++++++++++++++++----- doc/sphinx/src/governance.rst | 7 +- 2 files changed, 164 insertions(+), 32 deletions(-) diff --git a/doc/sphinx/src/contributing.rst b/doc/sphinx/src/contributing.rst index 86a6002a..89120b3f 100644 --- a/doc/sphinx/src/contributing.rst +++ b/doc/sphinx/src/contributing.rst @@ -1,13 +1,168 @@ +.. _singularity-eos: https://lanl.github.io/singularity-eos + Contributing ============================= -For the purpose of our development model, we label *Contributors* or -*Maintainers* of ``Phoebus``. Below we describe these labels and the -process for contributing to ``Phoebus``. +To contribute to ``Phoebus``, feel free to submit a pull +request or open an issue. + +1. Create a new fork of ``main`` where your changes can be made. +2. After completing, create a pull request, describe the final approach + and ensure the branch has no conflicts with current Regression Tests + and Formatting Checks. +3. Assign external reviewers (a minimum of 1, one of which should be a + Maintainer, and which cannot be the contributor). Provide concise + description of changes. +4. Once comments/feedback is addressed, the PR will be merged into the + main branch and changes will be added to list of changes in the next + release. +5. At present, Releases (with a git version tag) for the ``main`` branch + of ``Phoebus`` will occur at a 6 to 12 month cadence or following + implementation of a major enhancement or capability to the code base. + +*Maintainers* of ``Phoebus`` will follow the same process for +contributions as above except for the following differences: they may +create branches directly within ``Phoebus``, they will hold repository +admin privileges, and actively participate in the technical review of +contributions to ``Phoebus``. + + +Pull request protocol +---------------------- + +When submitting a pull request, there is a default template that is automatically +poluated. The pull request should sufficiently summarize all changes. +As necessary, tests should be added for new features of bugs fixed. + +Before a pull request will be merged, the code should be formatted. We +use clang-format for this, pinned to version 12. +The script ``scripts/bash/format.sh`` will apply ``clang-format`` +to C++ source files in the repository as well as ``black`` to python files, if available. +The script takes two CLI arguments +that may be useful, ``CFM``, which can be set to the path for your +clang-format binary, and ``VERBOSE``, which if set to ``1`` adds +useful output. For example: + +.. code-block:: bash + + CFM=clang-format-12 VERBOSE=1 ./scripts/bash/format.sh + +Test Suite +---------- + +Several sets of tests are triggered on a pull request: a static format +check, a docs buld, and a suite of unit and regression tests. +These are run through github's CPU infrastructure. These tests +help ensure that modifications to the code do not break existing capabilities +and ensure a consistent code style. + +Adding Tests +```````````` + +There are two primary categories of tests written in ``Phoebus``: + +* Unit tests +* Regression tests + +.. todo:: + + This section is incomplete. + +Expectations for code review +----------------------------- + +Much of what follows is adapted from `singularity-eos`_. + +From the perspective of the contributor +```````````````````````````````````````` + +Code review is an integral part of the development process +for ``Phoebus``. You can expect at least one, perhaps many, +core developers to read your code and offer suggestions. +You should treat this much like scientific or academic peer review. +You should listen to suggestions but also feel entitled to push back +if you believe the suggestions or comments are incorrect or +are requesting too much effort. + +Reviewers may offer conflicting advice, if this is the case, it's an +opportunity to open a discussion and communally arrive at a good +approach. You should feel empowered to argue for which of the +conflicting solutions you prefer or to suggest a compromise. If you +don't feel strongly, that's fine too, but it's best to say so to keep +the lines of communication open. + +Big contributions may be difficult to review in one piece and you may +be requested to split your pull request into two or more separate +contributions. You may also receive many "nitpicky" comments about +code style or structure. These comments help keep a broad codebase, +with many contributors uniform in style and maintainable with +consistent expectations accross the code base. While there is no +formal style guide for now, the regular contributors have a sense for +the broad style of the project. You should take these stylistic and +"nitpicky" suggestions seriously, but you should also feel free to +push back. + +As with any creative endeavor, we put a lot of ourselves into our +code. It can be painful to receive criticism on your contribution and +easy to take it personally. While you should resist the urge to take +offense, it is also partly code reviewer's responsiblity to create a +constructive environment, as discussed below. + +Expectations of code reviewers +```````````````````````````````` + +A good code review builds a contribution up, rather than tearing it +down. Here are a few rules to keep code reviews constructive and +congenial: + +* You should take the time needed to review a contribution and offer + meaningful advice. Unless a contribution is very small, limit + the times you simply click "approve" with a "looks good to me." + +* You should keep your comments constructive. For example, rather than + saying "this pattern is bad," try saying "at this point, you may + want to try this other pattern." + +* Avoid language that can be misconstrued, even if it's common + notation in the commnunity. For example, avoid phrases like "code + smell." + +* Explain why you make a suggestion. In addition to saying "try X + instead of Y" explain why you like pattern X more than pattern Y. + +* A contributor may push back on your suggestion. Be open to the + possibility that you're either asking too much or are incorrect in + this instance. Code review is an opportunity for everyone to learn. + +* Don't just highlight what you don't like. Also highlight the parts + of the pull request you do like and thank the contributor for their + effort. + +General principle for everyone +``````````````````````````````` + +It's hard to convey tone in text correspondance. Try to read what +others write favorably and try to write in such a way that your tone +can't be mis-interpreted as malicious. + +A Large Ecosystem +------------------------ + +``Phoebus`` depends on several other open-source, Los Alamos +maintained, projects. In particular, ``Parthenon``, ``singularity-eos``, +``singularity-opac``, ``spiner``, and ``kokkos``. +If you have issues with these projects, ideally +submit issues on the relevant github pages. However, if you can't +figure out where an issue belongs, no big deal. Submit where you can +and we'll engage with you to figure out how to proceed. Becoming a Contributor ---------------------- +For the purpose of our development model, we label *Contributors* or +*Maintainers* of ``Phoebus``. Below we describe these labels and the +process for contributing to ``Phoebus``. + We welcome contributions from scientists from a large variety of relativistic astrophysics. New users are welcome to contributions to ``Phoebus`` via the *Contributors* process. Contributors with 6 merged @@ -29,34 +184,10 @@ integration. To maintain their active status as Maintainer, all members will agree to adhere to our Community Code of Conduct and adhere to a Memorandum of -Understanding described `here `__. - -Contributing to ``Phoebus`` ------------------------------ - -**Contributors**: (process for most users of ``Phoebus``) +Understanding described :ref:`here `. -1. Create a new fork of ``main`` where your changes can be made. -2. After completing, create a pull request, describe the final approach - and ensure the branch has no conflicts with current Regression Tests - and Formatting Checks. -3. Assign external reviewers (a minimum of 1, one of which should be a - Maintainer, and which cannot be the contributor). Provide concise - description of changes. -4. Once comments/feedback is addressed, the PR will be merged into the - main branch and changes will be added to list of changes in the next - release. -5. At present, Releases (with a git version tag) for the ``main`` branch - of ``Phoebus`` will occur at a 6 to 12 month cadence or following - implementation of a major enhancement or capability to the code base. - -*Maintainers* of ``Phoebus`` will follow the same process for -contributions as above except for the following differences: they may -create branches directly within ``Phoebus``, they will hold repository -admin privileges, and actively participate in the technical review of -contributions to ``Phoebus``. -List of Current Maintainers of ``Phoebus`` +List of Current Maintainers of Phoebus ------------------------------------------ +-------------------+--------------------------------+-----------------------+ diff --git a/doc/sphinx/src/governance.rst b/doc/sphinx/src/governance.rst index 4abc519a..88b9ac5d 100644 --- a/doc/sphinx/src/governance.rst +++ b/doc/sphinx/src/governance.rst @@ -54,6 +54,9 @@ the repo and are working to keep the code sustainable long term. To maintain their active status as a User, Contributor, or Maintainer, all members will agree to adhere to a Code of Conduct. + +.. _mou: + Memorandum of Understanding --------------------------- @@ -64,14 +67,12 @@ the ``Phoebus`` Community requires the following: - Maintainers will uphold and exemplify the *Development Model* outlined as part of the ``phoebus`` Community described - `here `__. + :doc:`here `. - Maintainers will make a good-faith effort to attend team meetings. In an effort to accommodate the dynamic schedules and availability of many of the core team of *Maintainers*, coordination efforts will be made to ensure that *at least* 1 *Maintainer* is present for all meetings. -- Maintainers will make a good-faith effort to attend the annual - workshop. - Maintainers will make a good-faith effort to monitor the fraction of the meeting time that they speak. The meeting time is limited, and several people may have something interesting to say, report on their