Docs/unify contrib docs #9742
Docs/unify contrib docs #9742
Conversation
bdraco
added
bot:chronographer:skip
CodSpeed Performance ReportMerging #9742 will not alter performanceComparing Summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| .. code-block:: shell | ||
|
|
||
| $ brew install graphviz |
There was a problem hiding this comment.
Looks like this needs extra indentation.
| .. code-block:: shell | |
| $ brew install graphviz | |
| .. code-block:: shell | |
| $ brew install graphviz |
| Contributing | ||
| ============ | ||
| .. include:: ../CONTRIBUTING.rst | ||
| :end-before: export-point-instructions-for-contributors | ||
|
|
||
| (:doc:`contributing-admins`) |
There was a problem hiding this comment.
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
|
|
||
| .. _GitHub: https://github.com/aio-libs/aiohttp | ||
| .. export-point-instructions-for-contributors |
There was a problem hiding this comment.
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.
| @@ -1,36 +1,56 @@ | |||
| Contributing | |||
There was a problem hiding this comment.
Looks like this title is lost entirely. Why?
|
|
||
| 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). |
There was a problem hiding this comment.
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
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| Also, ensure that you have `npm | ||
| <https://docs.npmjs.com/downloading-and-installing-node-js-and-npm>`_ installed. |
There was a problem hiding this comment.
I think this is only for building llhttp, right? So, maybe:
| 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. |
| 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. |
There was a problem hiding this comment.
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`).
There was a problem hiding this comment.
As this is prerequisite, I'd move this step above the make doc command, so users see it first.
|
Thank you for the reviews. I should be able to look through and push the changes later today. |
|
@tab1tha hey, we talked about improving the changelog fragments instructions back at the sprint. I thought you submitted a PR, but I don't see it. So I suppose you must've kept the changes locally. Could you push them to Git? I remember there being some great ideas that I wanted to incorporate… |
Hi @webknjaz , Will also look into the comments on this pull request. I have some things to wrap up but you can expect to see the code by tomorrow evening! Thank you for reminding me ⭐ |
What do these changes do?
Edit onboarding documentation to
make docdoesn't work.It also goes ahead to unify
CONTRIBUTING.rstin root anddocs/contributing.rstsuch that there is now one source of truth. The preliminary documentation is housed in the github-rendered outward-facingCONTRIBUTING.rstand the extended documentation is housed indocs/contributing.rstwhich 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
CONTRIBUTORS.txtCHANGES/foldername 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 animproper 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 breakingchanges 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 buildprocess.
.packaging: Notes for downstreams about unobvious side effectsand 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 abovecategories.
Make sure to use full sentences with correct case and punctuation,
for example:
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.