From a614491070d443801c3e9a2a221ce85117bb667f Mon Sep 17 00:00:00 2001 From: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com> Date: Fri, 3 Nov 2023 08:21:41 +0100 Subject: [PATCH] Backport PR #27213: DOC: consolidated coding guide and added naming conventions table --- doc/api/next_api_changes/README.rst | 12 +- doc/devel/coding_guide.rst | 214 ++++++------------------- doc/devel/contribute.rst | 237 ++++++++++++++++++++++++---- doc/devel/development_setup.rst | 2 + doc/devel/development_workflow.rst | 17 ++ doc/users/next_whats_new/README.rst | 12 +- 6 files changed, 292 insertions(+), 202 deletions(-) diff --git a/doc/api/next_api_changes/README.rst b/doc/api/next_api_changes/README.rst index de494911e6b2..75e70b456eb9 100644 --- a/doc/api/next_api_changes/README.rst +++ b/doc/api/next_api_changes/README.rst @@ -22,11 +22,17 @@ author's initials. Typically, each change will get its own file, but you may also amend existing files when suitable. The overall goal is a comprehensible documentation of the changes. -Please avoid using references in section titles, as it causes links to be -confusing in the table of contents. Instead, ensure that a reference is -included in the descriptive text. A typical entry could look like this:: +A typical entry could look like this:: Locators ~~~~~~~~ The unused `Locator.autoscale()` method is deprecated (pass the axis limits to `Locator.view_limits()` instead). + +Please avoid using references in section titles, as it causes links to be +confusing in the table of contents. Instead, ensure that a reference is +included in the descriptive text. + +.. NOTE + Lines 5-30 of this file are include in :ref:`api_whats_new`; + therefore, please check the doc build after changing this file. diff --git a/doc/devel/coding_guide.rst b/doc/devel/coding_guide.rst index 0385d6d388f0..22873020f103 100644 --- a/doc/devel/coding_guide.rst +++ b/doc/devel/coding_guide.rst @@ -8,144 +8,82 @@ Pull request guidelines `__ are the mechanism for contributing to Matplotlib's code and documentation. -It is recommended to check that your contribution complies with the following -rules before submitting a pull request: +We value contributions from people with all levels of experience. In particular, +if this is your first PR not everything has to be perfect. We'll guide you +through the PR process. Nevertheless, please try to follow our guidelines as well +as you can to help make the PR process quick and smooth. If your pull request is +incomplete or a work-in-progress, please mark it as a `draft pull requests `_ +on GitHub and specify what feedback from the developers would be helpful. -* If your pull request addresses an issue, please use the title to describe the - issue (e.g. "Add ability to plot timedeltas") and mention the issue number - in the pull request description to ensure that a link is created to the - original issue (e.g. "Closes #8869" or "Fixes #8869"). This will ensure the - original issue mentioned is automatically closed when your PR is merged. See - `the GitHub documentation - `__ - for more details. - -* Formatting should follow the recommendations of PEP8_, as enforced by - flake8_. Matplotlib modifies PEP8 to extend the maximum line length to 88 - characters. You can check flake8 compliance from the command line with :: - - python -m pip install flake8 - flake8 /path/to/module.py - - or your editor may provide integration with it. Note that Matplotlib - intentionally does not use the black_ auto-formatter (1__), in particular due - to its inability to understand the semantics of mathematical expressions - (2__, 3__). - - .. _PEP8: https://www.python.org/dev/peps/pep-0008/ - .. _flake8: https://flake8.pycqa.org/ - .. _black: https://black.readthedocs.io/ - .. __: https://github.com/matplotlib/matplotlib/issues/18796 - .. __: https://github.com/psf/black/issues/148 - .. __: https://github.com/psf/black/issues/1984 - -* All public methods should have informative docstrings with sample usage when - appropriate. Use the :ref:`docstring standards `. - -* For high-level plotting functions, consider adding a simple example either in - the ``Example`` section of the docstring or the - :ref:`examples gallery `. - -* Changes (both new features and bugfixes) should have good test coverage. See - :ref:`testing` for more details. - -* Import the following modules using the standard scipy conventions:: +Please be patient with reviewers. We try our best to respond quickly, but we have +limited bandwidth. If there is no feedback within a couple of days, please ping +us by posting a comment to your PR or reaching out on a :ref:`communication channel ` - import numpy as np - import numpy.ma as ma - import matplotlib as mpl - import matplotlib.pyplot as plt - import matplotlib.cbook as cbook - import matplotlib.patches as mpatches - In general, Matplotlib modules should **not** import `.rcParams` using ``from - matplotlib import rcParams``, but rather access it as ``mpl.rcParams``. This - is because some modules are imported very early, before the `.rcParams` - singleton is constructed. - -* If your change is a major new feature, add an entry to the ``What's new`` - section by adding a new file in ``doc/users/next_whats_new`` (see - :file:`doc/users/next_whats_new/README.rst` for more information). +Summary for pull request authors +================================ -* If you change the API in a backward-incompatible way, please document it in - :file:`doc/api/next_api_changes/behavior`, by adding a new file with the - naming convention ``99999-ABC.rst`` where the pull request number is followed - by the contributor's initials. (see :file:`doc/api/api_changes.rst` for more - information) +We recommend that you check that your contribution complies with the following +guidelines before submitting a pull request: -* If you add new public API or change public API, update or add the - corresponding type hints. Most often this is found in the corresponding - ``.pyi`` file for the ``.py`` file which was edited. Changes in ``pyplot.py`` - are type hinted inline. +.. rst-class:: checklist -* See below for additional points about :ref:`keyword-argument-processing`, if - applicable for your pull request. +* Changes, both new features and bugfixes, should have good test coverage. See + :ref:`testing` for more details. -.. note:: +* Update the :ref:`documentation ` if necessary. - The current state of the Matplotlib code base is not compliant with all - of these guidelines, but we expect that enforcing these constraints on all - new contributions will move the overall code base quality in the right - direction. +* All public methods should have informative docstrings with sample usage when + appropriate. Use the :ref:`docstring standards `. +* For high-level plotting functions, consider adding a small example to the + :ref:`examples gallery `. -.. seealso:: +* If you add a major new feature or change the API in a backward-incompatible + way, please document it as described in :ref:`new-changed-api` - * :ref:`coding_guidelines` - * :ref:`testing` - * :ref:`documenting-matplotlib` +* Code should follow our conventions as documented in our :ref:`coding_guidelines` +* When adding or changing public function signatures, add :ref:`type hints ` +* When adding keyword arguments, see our guide to :ref:`keyword-argument-processing`. -Summary for pull request authors -================================ +When opening a pull request on Github, please ensure that: -.. note:: +.. rst-class:: checklist - * We value contributions from people with all levels of experience. In - particular if this is your first PR not everything has to be perfect. - We'll guide you through the PR process. - * Nevertheless, please try to follow the guidelines below as well as you can to - help make the PR process quick and smooth. - * Be patient with reviewers. We try our best to respond quickly, but we - have limited bandwidth. If there is no feedback within a couple of days, - please ping us by posting a comment to your PR. +* Changes were made on a :ref:`feature branch `. -When making a PR, pay attention to: +* :ref:`pre-commit ` checks for spelling, formatting, etc pass -.. rst-class:: checklist +* The pull request targets the :ref:`main branch ` -* :ref:`Target the main branch `. -* Adhere to the :ref:`coding_guidelines`. -* Update the :ref:`documentation ` if necessary. -* Aim at making the PR as "ready-to-go" as you can. This helps to speed up - the review process. -* It is ok to open incomplete or work-in-progress PRs if you need help or - feedback from the developers. You may mark these as - `draft pull requests `_ - on GitHub. -* When updating your PR, instead of adding new commits to fix something, please - consider amending your initial commit(s) to keep the history clean. - You can achieve this by using +* If your pull request addresses an issue, please use the title to describe the + issue (e.g. "Add ability to plot timedeltas") and mention the issue number + in the pull request description to ensure that a link is created to the + original issue (e.g. "Closes #8869" or "Fixes #8869"). This will ensure the + original issue mentioned is automatically closed when your PR is merged. For more + details, see `linking an issue and pull request `__. - .. code-block:: bash +* :ref:`pr-automated-tests` pass - git commit --amend --no-edit - git push [your-remote-repo] [your-branch] --force-with-lease +For guidance on creating and managing a pull request, please see our +:ref:`contributing ` and :ref:`pull request workflow ` +guides. -See also :ref:`contributing` for how to make a PR. Summary for pull request reviewers ================================== .. redirect-from:: /devel/maintainer_workflow -.. note:: +**Please help review and merge PRs!** + +If you have commit rights, then you are trusted to use them. Please be patient +and `kind `__ with contributors. - * If you have commit rights, then you are trusted to use them. - **Please help review and merge PRs!** - * Be patient and `kind `__ with - contributors. +When reviewing, please ensure that the pull request satisfies the following +requirements before merging it: Content topics: @@ -196,61 +134,6 @@ Documentation * See :ref:`documenting-matplotlib` for our documentation style guide. -.. _release_notes: - -New features and API changes -^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -When adding a major new feature or changing the API in a backward incompatible -way, please document it by including a versioning directive in the docstring -and adding an entry to the folder for either the what's new or API change notes. - -+-------------------+-----------------------------+----------------------------------+ -| for this addition | include this directive | create entry in this folder | -+===================+=============================+==================================+ -| new feature | ``.. versionadded:: 3.N`` | :file:`doc/users/next_whats_new/`| -+-------------------+-----------------------------+----------------------------------+ -| API change | ``.. versionchanged:: 3.N`` | :file:`doc/api/next_api_changes/`| -| | | | -| | | probably in ``behavior/`` | -+-------------------+-----------------------------+----------------------------------+ - -The directives should be placed at the end of a description block. For example:: - - class Foo: - """ - This is the summary. - - Followed by a longer description block. - - Consisting of multiple lines and paragraphs. - - .. versionadded:: 3.5 - - Parameters - ---------- - a : int - The first parameter. - b: bool, default: False - This was added later. - - .. versionadded:: 3.6 - """ - - def set_b(b): - """ - Set b. - - .. versionadded:: 3.6 - - Parameters - ---------- - b: bool - -For classes and functions, the directive should be placed before the -*Parameters* section. For parameters, the directive should be placed at the -end of the parameter description. The patch release version is omitted and -the directive should not be added to entire modules. - .. _pr-labels: Labels @@ -330,7 +213,8 @@ Merging Automated tests --------------- -Before being merged, a PR should pass the :ref:`automated-tests`. If you are unsure why a test is failing, ask on the PR or in our `chat space `_ +Before being merged, a PR should pass the :ref:`automated-tests`. If you are +unsure why a test is failing, ask on the PR or in our :ref:`communication-channels` .. _pr-squashing: diff --git a/doc/devel/contribute.rst b/doc/devel/contribute.rst index a9bfb0f816dd..ee78beca0781 100644 --- a/doc/devel/contribute.rst +++ b/doc/devel/contribute.rst @@ -367,27 +367,160 @@ project that leads to a scientific publication, please follow the Coding guidelines ================= -API changes ------------ +While the current state of the Matplotlib code base is not compliant with all +of these guidelines, our goal in enforcing these constraints on new +contributions is that it improves the readability and consistency of the code base +going forward. -API consistency and stability are of great value. Therefore, API changes -(e.g. signature changes, behavior changes, removals) will only be conducted -if the added benefit is worth the user effort for adapting. +PEP8, as enforced by flake8 +--------------------------- + +Formatting should follow the recommendations of PEP8_, as enforced by flake8_. +Matplotlib modifies PEP8 to extend the maximum line length to 88 +characters. You can check flake8 compliance from the command line with :: + + python -m pip install flake8 + flake8 /path/to/module.py + +or your editor may provide integration with it. Note that Matplotlib intentionally +does not use the black_ auto-formatter (1__), in particular due to its inability +to understand the semantics of mathematical expressions (2__, 3__). + +.. _PEP8: https://www.python.org/dev/peps/pep-0008/ +.. _flake8: https://flake8.pycqa.org/ +.. _black: https://black.readthedocs.io/ +.. __: https://github.com/matplotlib/matplotlib/issues/18796 +.. __: https://github.com/psf/black/issues/148 +.. __: https://github.com/psf/black/issues/1984 + + +Package imports +--------------- +Import the following modules using the standard scipy conventions:: + + import numpy as np + import numpy.ma as ma + import matplotlib as mpl + import matplotlib.pyplot as plt + import matplotlib.cbook as cbook + import matplotlib.patches as mpatches + +In general, Matplotlib modules should **not** import `.rcParams` using ``from +matplotlib import rcParams``, but rather access it as ``mpl.rcParams``. This +is because some modules are imported very early, before the `.rcParams` +singleton is constructed. + +Variable names +-------------- + +When feasible, please use our internal variable naming convention for objects +of a given class and objects of any child class: + ++------------------------------------+---------------+------------------------------------------+ +| base class | variable | multiples | ++====================================+===============+==========================================+ +| `~matplotlib.figure.FigureBase` | ``fig`` | | ++------------------------------------+---------------+------------------------------------------+ +| `~matplotlib.axes.Axes` | ``ax`` | | ++------------------------------------+---------------+------------------------------------------+ +| `~matplotlib.transforms.Transform` | ``trans`` | ``trans__`` | ++ + + + +| | | ``trans_`` when target is screen | ++------------------------------------+---------------+------------------------------------------+ + +Generally, denote more than one instance of the same class by adding suffixes to +the variable names. If a format isn't specified in the table, use numbers or +letters as appropriate. + + +.. _type-hints: + +Type hints +---------- + +If you add new public API or change public API, update or add the +corresponding `mypy `_ type hints. +We generally use `stub files +`_ +(``*.pyi``) to store the type information; for example ``colors.pyi`` contains +the type information for ``colors.py``. A notable exception is ``pyplot.py``, +which is type hinted inline. + +Type hints are checked by the mypy :ref:`pre-commit hook ` +and can often be verified using ``tools\stubtest.py`` and occasionally may +require the use of ``tools\check_typehints.py``. + + +.. _new-changed-api: -Because we are a visualization library our primary output is the final -visualization the user sees. Thus it is our :ref:`long standing -` policy that the appearance of the figure is part of the API -and any changes, either semantic or esthetic, will be treated as a -backwards-incompatible API change. +API changes and new features +---------------------------- +API consistency and stability are of great value; Therefore, API changes +(e.g. signature changes, behavior changes, removals) will only be conducted +if the added benefit is worth the effort of adapting existing code. + +Because we are a visualization library, our primary output is the final +visualization the user sees; therefore, the appearance of the figure is part of +the API and any changes, either semantic or :ref:`esthetic `, +are backwards-incompatible API changes. + +.. _api_whats_new: + +Announce changes, deprecations, and new features +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +When adding or changing the API in a backward in-compatible way, please add the +appropriate :ref:`versioning directive ` and document it +for the release notes and add the entry to the appropriate folder: + ++-------------------+-----------------------------+----------------------------------------------+ +| addition | versioning directive | announcement folder | ++===================+=============================+==============================================+ +| new feature | ``.. versionadded:: 3.N`` | :file:`doc/users/next_whats_new/` | ++-------------------+-----------------------------+----------------------------------------------+ +| API change | ``.. versionchanged:: 3.N`` | :file:`doc/api/next_api_changes/[kind]` | ++-------------------+-----------------------------+----------------------------------------------+ + +API deprecations are first introduced and then expired. During the introduction +period, users are warned that the API *will* change in the future. +During the expiration period, code is changed as described in the notice posted +during the introductory period. + ++-----------+--------------------------------------------------+----------------------------------------------+ +| stage | required changes | announcement folder | ++===========+==================================================+==============================================+ +| introduce | :ref:`introduce deprecation ` | :file:`doc/api/next_api_changes/deprecation` | ++-----------+--------------------------------------------------+----------------------------------------------+ +| expire | :ref:`expire deprecation ` | :file:`doc/api/next_api_changes/[kind]` | ++-----------+--------------------------------------------------+----------------------------------------------+ + +For both change notes and what's new, please avoid using references in section +titles, as it causes links to be confusing in the table of contents. Instead, +ensure that a reference is included in the descriptive text. + +API Change Notes +"""""""""""""""" +.. include:: ../api/next_api_changes/README.rst + :start-line: 5 + :end-line: 31 + +What's new +"""""""""" +.. include:: ../users/next_whats_new/README.rst + :start-line: 5 + :end-line: 24 + + +Deprecation +^^^^^^^^^^^ API changes in Matplotlib have to be performed following the deprecation process -below, except in very rare circumstances as deemed necessary by the development team. -This ensures that users are notified before the change will take effect and thus -prevents unexpected breaking of code. +below, except in very rare circumstances as deemed necessary by the development +team. This ensures that users are notified before the change will take effect +and thus prevents unexpected breaking of code. Rules -^^^^^ - +""""" - Deprecations are targeted at the next point.release (e.g. 3.x) - Deprecated API is generally removed two point-releases after introduction of the deprecation. Longer deprecations can be imposed by core developers on @@ -398,12 +531,12 @@ Rules - If in doubt, decisions about API changes are finally made by the API consistency lead developer -Introducing -^^^^^^^^^^^ +.. _intro-deprecation: + +Introduce deprecation +""""""""""""""""""""" -#. Announce the deprecation in a new file - :file:`doc/api/next_api_changes/deprecations/99999-ABC.rst` where ``99999`` - is the pull request number and ``ABC`` are the contributor's initials. +#. Create :ref:`deprecation notice ` #. If possible, issue a `~matplotlib.MatplotlibDeprecationWarning` when the deprecated API is used. There are a number of helper tools for this: @@ -439,16 +572,13 @@ Introducing :file:`ci/mypy-stubtest-allowlist.txt` under a heading indicating the deprecation version number. -Expiring -^^^^^^^^ +.. _expire-deprecation: + +Expire deprecation +"""""""""""""""""" -#. Announce the API changes in a new file - :file:`doc/api/next_api_changes/[kind]/99999-ABC.rst` where ``99999`` - is the pull request number and ``ABC`` are the contributor's initials, and - ``[kind]`` is one of the folders :file:`behavior`, :file:`development`, - :file:`removals`. See :file:`doc/api/next_api_changes/README.rst` for more - information. For the content, you can usually copy the deprecation notice - and adapt it slightly. +#. Create :ref:`deprecation announcement `. For the content, + you can usually copy the deprecation notice and adapt it slightly. #. Change the code functionality and remove any related deprecation warnings. @@ -466,8 +596,8 @@ Expiring require that mechanism for deprecation. For removed items that were not in the stub file, only deleting from the allowlist is required. -Adding new API --------------- +Adding new API and features +^^^^^^^^^^^^^^^^^^^^^^^^^^^ Every new function, parameter and attribute that is not explicitly marked as private (i.e., starts with an underscore) becomes part of Matplotlib's public @@ -485,6 +615,51 @@ take particular care when adding new API: __ https://emptysqua.re/blog/api-evolution-the-right-way/#adding-parameters +.. _versioning-directives: + +Versioning directives +""""""""""""""""""""" + +When making a backward incompatible change, please add a versioning directive in +the docstring. The directives should be placed at the end of a description block. +For example:: + + class Foo: + """ + This is the summary. + + Followed by a longer description block. + + Consisting of multiple lines and paragraphs. + + .. versionadded:: 3.5 + + Parameters + ---------- + a : int + The first parameter. + b: bool, default: False + This was added later. + + .. versionadded:: 3.6 + """ + + def set_b(b): + """ + Set b. + + .. versionadded:: 3.6 + + Parameters + ---------- + b: bool + +For classes and functions, the directive should be placed before the +*Parameters* section. For parameters, the directive should be placed at the +end of the parameter description. The patch release version is omitted and +the directive should not be added to entire modules. + + New modules and files: installation ----------------------------------- diff --git a/doc/devel/development_setup.rst b/doc/devel/development_setup.rst index 9da1c5737a36..e18d85cebde0 100644 --- a/doc/devel/development_setup.rst +++ b/doc/devel/development_setup.rst @@ -195,6 +195,8 @@ so that when you make code or document related changes you are aware of the exis * Run test cases to verify installation :ref:`testing` * Verify documentation build :ref:`documenting-matplotlib` +.. _pre-commit-hooks: + Install pre-commit hooks ======================== `pre-commit `_ hooks save time in the review process by diff --git a/doc/devel/development_workflow.rst b/doc/devel/development_workflow.rst index 50170ee4ade3..3f4fe956e37e 100644 --- a/doc/devel/development_workflow.rst +++ b/doc/devel/development_workflow.rst @@ -151,6 +151,23 @@ If you don't think your request is ready to be merged, just say so in your pull request message and use the "Draft PR" feature of GitHub. This is a good way of getting some preliminary code review. +.. _update-pull-request: + +Update a pull request +===================== + +When updating your pull request after making revisions, instead of adding new +commits, please consider amending your initial commit(s) to keep the commit +history clean. + +You can achieve this by using + + .. code-block:: bash + + git commit -a --amend --no-edit + git push [your-remote-repo] [your-branch] --force-with-lease + + Manage commit history ===================== diff --git a/doc/users/next_whats_new/README.rst b/doc/users/next_whats_new/README.rst index e1b27ef97f1e..98b601ee32d8 100644 --- a/doc/users/next_whats_new/README.rst +++ b/doc/users/next_whats_new/README.rst @@ -11,9 +11,7 @@ something like :file:`cool_new_feature.rst` if you have added a brand new feature or something like :file:`updated_feature.rst` for extensions of existing features. -Please avoid using references in section titles, as it causes links to be -confusing in the table of contents. Instead, ensure that a reference is -included in the descriptive text. Include contents of the form: :: +Include contents of the form:: Section title for feature ------------------------- @@ -23,3 +21,11 @@ included in the descriptive text. Include contents of the form: :: A sub-section ~~~~~~~~~~~~~ + +Please avoid using references in section titles, as it causes links to be +confusing in the table of contents. Instead, ensure that a reference is +included in the descriptive text. + +.. NOTE + Lines 5-24 of this file are include in :ref:`api_whats_new`; + therefore, please check the doc build after changing this file.