Docs/unify contrib docs #9742

tab1tha · 2024-11-09T16:23:25Z

What do these changes do?

Edit onboarding documentation to

emphasize the need for npm as a pre-requisite to run tests,
make the git clone link explicit like the rest of the commands,
point to GitHub docs if user isn't familiar yet,
inform macOS users about the possible need for graphviz if make doc doesn't work.

It also goes ahead to unify CONTRIBUTING.rst in root and docs/contributing.rst such that there is now one source of truth. The preliminary documentation is housed in the github-rendered outward-facing CONTRIBUTING.rst and the extended documentation is housed in docs/contributing.rst which is rendered on readthedocs.

The changes in this pull requests remove the previous duplication of preliminary documentation such that further changes to it will be done in one place (CONTRIBUTING.rst) and imported to the other locations as needed.

Are there changes in behavior for the user?

None

Is it a substantial burden for the maintainers to support this?

None. This makes future maintenance easier.

Related issue number

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES/ folder
- name it <issue_or_pr_num>.<type>.rst (e.g. 588.bugfix.rst)
- if you don't have an issue number, change it to the pull request
  number after creating the PR
  - .bugfix: A bug fix for something the maintainers deemed an
    improper undesired behavior that got corrected to match
    pre-agreed expectations.
  - .feature: A new behavior, public APIs. That sort of stuff.
  - .deprecation: A declaration of future API removals and breaking
    changes in behavior.
  - .breaking: When something public is removed in a breaking way.
    Could be deprecated in an earlier release.
  - .doc: Notable updates to the documentation structure or build
    process.
  - .packaging: Notes for downstreams about unobvious side effects
    and tooling. Changes in the test invocation considerations and
    runtime assumptions.
  - .contrib: Stuff that affects the contributor experience. e.g.
    Running tests, building the docs, setting up the development
    environment.
  - .misc: Changes that are hard to assign to any of the above
    categories.
- Make sure to use full sentences with correct case and punctuation,
  for example:
```
Fixed issue with non-ascii contents in doctest text files
-- by :user:`contributor-gh-handle`.
```
  Use the past tense or the present tense a non-imperative mood,
  referring to what's changed compared to the last released version
  of this project.

codspeed-hq · 2024-11-09T16:26:36Z

CodSpeed Performance Report

Merging #9742 will not alter performance

_{Comparing tab1tha:docs/unify-contrib-docs (7dbfeca) with master (5ddeb06)}

Summary

✅ 14 untouched benchmarks

codecov · 2024-11-09T16:45:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.67%. Comparing base (5ddeb06) to head (7dbfeca).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9742      +/-   ##
==========================================
+ Coverage   98.60%   98.67%   +0.06%     
==========================================
  Files         117      117              
  Lines       35807    35807              
  Branches     4250     4250              
==========================================
+ Hits        35307    35331      +24     
+ Misses        340      320      -20     
+ Partials      160      156       -4

Flag	Coverage Δ
CI-GHA	`98.55% <ø> (+0.05%)`	⬆️
OS-Linux	`98.24% <ø> (+0.05%)`	⬆️
OS-Windows	`95.99% <ø> (ø)`
OS-macOS	`97.30% <ø> (-0.01%)`	⬇️
Py-3.10.11	`95.55% <ø> (-1.61%)`	⬇️
Py-3.10.15	`97.72% <ø> (ø)`
Py-3.11.10	`97.78% <ø> (+<0.01%)`	⬆️
Py-3.11.9	`97.22% <ø> (ø)`
Py-3.12.7	`98.26% <ø> (ø)`
Py-3.13.0	`98.24% <ø> (+0.88%)`	⬆️
Py-3.9.13	`97.07% <ø> (ø)`
Py-3.9.20	`97.63% <ø> (ø)`
Py-pypy7.3.16	`97.26% <ø> (?)`
VM-macos	`97.30% <ø> (-0.01%)`	⬇️
VM-ubuntu	`98.24% <ø> (+0.05%)`	⬆️
VM-windows	`95.99% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

webknjaz

It looks like Sphinx isn't set up in a fully strict mode, which means that some things don't fail loudly in CI. Here's a few places that I noticed needing improvements.

webknjaz · 2024-11-10T02:02:32Z

docs/contributing.rst

+   .. code-block:: shell
+
+      $ brew install graphviz


Looks like this needs extra indentation.

Suggested change

.. code-block:: shell

$ brew install graphviz

.. code-block:: shell

$ brew install graphviz

webknjaz · 2024-11-10T02:04:31Z

docs/contributing.rst

-Contributing
-============
+.. include:: ../CONTRIBUTING.rst
+   :end-before: export-point-instructions-for-contributors

 (:doc:`contributing-admins`)


Why not keep this in the same place: at the top of the document?

https://aiohttp--9742.org.readthedocs.build/en/9742/contributing.html#instructions-for-contributors

webknjaz · 2024-11-10T02:05:32Z

CONTRIBUTING.rst


-.. _GitHub: https://github.com/aio-libs/aiohttp
+.. export-point-instructions-for-contributors


Why is this comment at the end and isn't splitting the doc closer to the top? It doesn't do anything in this position, really.

webknjaz · 2024-11-10T02:05:56Z

CONTRIBUTING.rst

@@ -1,36 +1,56 @@
-Contributing


Looks like this title is lost entirely. Why?

webknjaz · 2024-11-10T02:13:14Z

CONTRIBUTING.rst


  2. Setup your machine with the required development environment

  3. Make a change

  4. Make sure all tests passed

-  5. Add a file into the ``CHANGES`` folder, named after the ticket or PR number
+  5. Add a file into the ``CHANGES`` folder (see `Changelog update <CHANGES>`_ for how).


This relative file reference only works on GitHub and is a broken link in Sphinx: https://aiohttp--9742.org.readthedocs.build/en/9742/contributing.html#instructions-for-contributors

Dreamsorcerer · 2024-11-10T16:32:17Z

CONTRIBUTING.rst

+   GitHub issue and pull request threads are automatically locked when there has
+   not been any recent activity for one year.  Please open a `new issue


I don't think this is true anymore (I see locked issues from years ago, but don't think there's any auto-locking in the past 4 years). Maybe just reword to open a new issue instead of commenting on a closed issue.

Or we should re-establish the automation. It used to be run by https://github.com/apps/stale, but the app has been deprecated in favor of the action: https://github.com/marketplace/actions/close-stale-issues. We just never remembered to migrate.

Personally, I find auto-locked issues a little annoying. Though if it's after a year then I don't think it makes a difference, we don't tend to see anything on issues that have been closed that long. For this PR, I think it just needs to document the current status.

Dreamsorcerer · 2024-11-10T16:34:23Z

docs/contributing.rst

+Also, ensure that you have `npm
+<https://docs.npmjs.com/downloading-and-installing-node-js-and-npm>`_ installed.


I think this is only for building llhttp, right? So, maybe:

Suggested change

Also, ensure that you have `npm

<https://docs.npmjs.com/downloading-and-installing-node-js-and-npm>`_ installed.

If you intend to run the parser tests, ensure that you have `npm

<https://docs.npmjs.com/downloading-and-installing-node-js-and-npm>`_ installed.

Dreamsorcerer · 2024-11-10T16:37:54Z

docs/contributing.rst

+Note that this page doesn't automatically refresh itself. So, if you make more changes, build the docs again to see how they look on the page.
+
+.. note::
+   If you are on MacOS, you might need to install `graphviz <https://graphviz.org/>`_ first.


This isn't limited to MacOS, the PR added an apt package to make this work in the CI:
https://github.com/aio-libs/aiohttp/pull/9359/files#diff-cde814ef2f549dc093f5b8fc533b7e8f47e7b32a8081e0760e57d5c25a1139d9R17-R18

So, maybe something like:

You'll also need to install graphviz with your package manager (e.g. `apt install graphviz` or `brew install graphviz`).

As this is prerequisite, I'd move this step above the make doc command, so users see it first.

tab1tha · 2024-11-11T06:37:56Z

Thank you for the reviews. I should be able to look through and push the changes later today.

Tambe Tabitha Achere added 3 commits November 9, 2024 15:39

git clone command. graphviz note

2f22b46

unify contribution docs

e8f3790

add Tambe T to contributors.txt

7dbfeca

tab1tha requested review from webknjaz and asvetlov as code owners November 9, 2024 16:23

bdraco added bot:chronographer:skip This PR does not need to include a change note backport-3.10 Trigger automatic backporting to the 3.10 release branch by Patchback robot backport-3.11 Trigger automatic backporting to the 3.11 release branch by Patchback robot labels Nov 9, 2024

bdraco closed this Nov 9, 2024

bdraco reopened this Nov 9, 2024

webknjaz reviewed Nov 10, 2024

View reviewed changes

Dreamsorcerer reviewed Nov 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docs/unify contrib docs #9742

Docs/unify contrib docs #9742

tab1tha commented Nov 9, 2024 •

edited

Loading

codspeed-hq bot commented Nov 9, 2024 •

edited

Loading

codecov bot commented Nov 9, 2024 •

edited

Loading

webknjaz left a comment

webknjaz Nov 10, 2024

webknjaz Nov 10, 2024

webknjaz Nov 10, 2024

webknjaz Nov 10, 2024

webknjaz Nov 10, 2024

Dreamsorcerer Nov 10, 2024

webknjaz Nov 14, 2024

Dreamsorcerer Nov 14, 2024

Dreamsorcerer Nov 10, 2024 •

edited

Loading

Dreamsorcerer Nov 10, 2024 •

edited

Loading

Dreamsorcerer Nov 10, 2024

tab1tha commented Nov 11, 2024


		.. _GitHub: https://github.com/aio-libs/aiohttp
		.. export-point-instructions-for-contributors

		GitHub issue and pull request threads are automatically locked when there has
		not been any recent activity for one year. Please open a `new issue

		Also, ensure that you have `npm
		<https://docs.npmjs.com/downloading-and-installing-node-js-and-npm>`_ installed.

Docs/unify contrib docs #9742

Are you sure you want to change the base?

Docs/unify contrib docs #9742

Conversation

tab1tha commented Nov 9, 2024 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Is it a substantial burden for the maintainers to support this?

Related issue number

Checklist

codspeed-hq bot commented Nov 9, 2024 • edited Loading

Merging #9742 will not alter performance

Summary

codecov bot commented Nov 9, 2024 • edited Loading

Codecov Report

webknjaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dreamsorcerer Nov 10, 2024 • edited Loading

Choose a reason for hiding this comment

Dreamsorcerer Nov 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tab1tha commented Nov 11, 2024

tab1tha commented Nov 9, 2024 •

edited

Loading

codspeed-hq bot commented Nov 9, 2024 •

edited

Loading

codecov bot commented Nov 9, 2024 •

edited

Loading

Dreamsorcerer Nov 10, 2024 •

edited

Loading

Dreamsorcerer Nov 10, 2024 •

edited

Loading