Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reorganize docs v2 #2174

Merged
merged 24 commits into from May 8, 2021
Merged

Reorganize docs v2 #2174

merged 24 commits into from May 8, 2021

Conversation

ichard26
Copy link
Collaborator

@ichard26 ichard26 commented May 1, 2021

Resolves GH-1759.


I'm finally done 🎉

To anyone trying to review this, I recommend looking at the rendered previews I've provided below OR doing this commit-by-commit (looking at the whole diff will be painful). I've tried to provide a decent commit history but yeah, my apologies. Take your time and don't worry about asking for a lot of changes. I want this structure to last us a long time.

I'd ideally want as many reviews as possible but as a minimum I'd like two maintainer and one user reviews.

What I'm looking for in terms of feedback and reviews:

  • Does the structure make sense?
    • Can you navigate it and not end up confused?
  • Is there any important information I'm missing (I do plan to write a bunch of new docs after I'm done with this but it's good to know early just in case some structural changes have to happen)
  • Do you think this structure is expandable and flexible enough? (i.e. will it last us a long time?)


FYI, I want to handle merging this myself since I'd like to edit the commit message to my liking.

Also I'll need this to be merged as close as possible before a release since the README.md / CONTRIBUTING.md links are dependent on the stable version on RTD having these changes at the same time.

recommonmark is being deprecated in favour of MyST-Parser. This change
is welcomed, especially since MyST-Parser has syntax extensions for the
Commonmark standard. Effectively we get to use a language that's powerful
and expressive like ReST, but get the simplicity of Markdown.

The rest of this effort will be using some MyST features.

This reorganization efforts aims to remove as much duplication as possible.
The regeneration step once needed is gone, significantly simplifing our
Sphinx documentation configuration.
Also update `docs/requirements.txt`
prettier doesn't support MyST's syntax extensions which are going to be
used in this reorganization effort so we have to switch formatter.

Unfortanately mdformat's style is different from prettier's so time to
reformat the whole repo too.

We're excluding .github/ISSUE_TEMPLATE because I have no idea whether
its changes are safe, so let's play it safe.
MyST-Parser / sphinx's linkcheck complains otherwise.
They're for contributors of Black anyway. Also added a note in the
summary document warning about the lack of attention the reference has
been dealing with.
- add some more detail about Black's beta status
- add licensing info
- add external links in the main TOC for GitHub, PyPI, and IRC
- prepare main TOC for new structure
Not only was the AUTHORS list quite long, this makes it easy to include
it in the Sphinx docs with just a simple symlink.
Yes the document is orphaned but it is linked to in the landing page
(docs/index.rst).
This mostly was a restructuring commit, there has been a few updates but
not many. The main goal was to split "current style" and "planned
changes to the style that haven't happened yet" to avoid confusion.
This is basically a quick start + even more. This commit is certainly
one of most creatively involved in this effort.
This commit was as much restructuring as new content. Instead of being
in one giant file, usage and configuration documentation can expand
without bloating a single file.
Just a restructuring commit ...
This is a promising area of documentation that could easily grow in the
future, let's prepare for that!
This is also another area that I expect to see significant growth in.
Contributors to Black could definitely do with some more specific docs
that clears up certain parts of our slightly confusing project (it's
only confusing because we're getting big and old!).
@ichard26 ichard26 added T: documentation Improvements to the docs (e.g. new topic, correction, etc) C: maintenance Related to project maintenance, e.g. CI, testing, policy changes, releases labels May 1, 2021
@ichard26
Copy link
Collaborator Author

ichard26 commented May 1, 2021

/cc @felix-hilden you've been quite interested and helpful with the docs lately (thank you btw!), so I was wondering you'd like to do an user checkover / review and provide *some* feedback. Now don't worry, I don't expect you to do a thorough review but I'd like as much feedback as possible. Also totally understand if you don't want to!

@JelleZijlstra
Copy link
Collaborator

Thanks so much! I'm going to take this occasion to just read the whole docs and comment on anything that seems off.

Those are mostly pretty minor. Overall this makes the docs a lot better and I hope we'll get a release with these reorganized docs soon!

.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@felix-hilden
Copy link
Collaborator

Would be my pleasure! First of all, it all looks so good. Worlds apart with the current structure 👌

Jelle commented on quite a few points already:

  * "Black doesn’t provide many options" – only 20 of them. I don't think we can make this claim with a straight face any more.

Perhaps the intent could still be communicated to the users. Something like: "Black is opinionated, so style configuration options are deliberately limited and rarely added".

* https://ichard26-testblackdocs.readthedocs.io/en/reorganize-docs-v2/change_log.html
  
  * It'd be nice if the PR numbers autolinked to the PR on GitHub

See #2087 and maybe discuss there? Doesn't necessarily have to be RST!

  • Getting started: "Installing from GitHub could be removed, as the section is very short and right after "Installation".
  • Getting started: "Next steps" has many sentences in individual paragraphs. That's pretty jarring to read. Have them in a single paragraph?

Having the contributing docs online is a great change too. In line with that, should the change log be single sourced as well? Now the readme links to the GitHub tree, but it could be changed to a RTD link (because the RTD latest has changes from master I presume). And even further: the readme duplicates a lot of information on the documentation. Does it need to? If users are directed to RTD from everywhere, the GitHub readme can serve contributors and the repository more, not users.

I'm not sure if all of these changes are appropriate for this PR 😅 I agree: this is great, and after fixing the small details it surely is good to go as an incremental improvement (read: giant leap for mankind). All the rest can be broken down into future contributions.

@ichard26
Copy link
Collaborator Author

ichard26 commented May 1, 2021

Haha, I've been reading over my own work and I see a bunch of little errors too :)

I tried to avoid writing too much new content / editing existing content (other than the Getting Started page plus all of the section index pages) so this wouldn't be as painful to review. So IMO unless the suggestion falls under something I (re)wrote, I'll add them in a separate PR.


Perhaps the intent could still be communicated to the users. Something like: "Black is opinionated, so style configuration options are deliberately limited and rarely added".

👍

Getting started: "Installing from GitHub could be removed, as the section is very short and right after "Installation".

IDC but sure.

Getting started: "Next steps" has many sentences in individual paragraphs. That's pretty jarring to read. Have them in a single paragraph?

👍

In line with that, should the change log be single sourced as well?

👍

the readme duplicates a lot of information on the documentation. Does it need to?

We've already tried hard to get rid as much duplicated content as possible, but I didn't think I would be able to get approval to strip 98% of the README content 🙃 But in general I agree it shouldn't and I'll take another look seeing what else I should remove.

@felix-hilden
Copy link
Collaborator

Yeah there's definitely an argument to be had about not simply having a "users go to RTD" and nothing else. I like it that way, but I imagine there are lots of people who prefer to have something resembling what's currently there. And again, incremental changes :D

@wbolster
Copy link
Contributor

wbolster commented May 4, 2021

quick feedback

the editor integration for emacs should point to python-black.el at https://github.com/wbolster/emacs-python-black

the link to "reformatter.el" should go, since it's a low level library to build integrations, and python-black.el uses it.

🙏

@ichard26
Copy link
Collaborator Author

ichard26 commented May 4, 2021

@wbolster noted, I'll take of it soon (or eventually). Thank you for noticing and reporting!

I know I said I wanted to do these after landing this but given there's
going to be no time between this being merged and a release getting
pushed, I want these changes to make it in.

- drop the number flag for mdformat - to reduce diffs, see also:
  https://mdformat.readthedocs.io/en/stable/users/style.html#ordered-lists
- the GH issue templates should be safe by mdformat, so get rid of the
  exclude
- clarify our configuration position - i.e. stop claiming we don't have
  many options, instead say we want as little formatting knobs as
  possible
- lots and lots of punctuation, spelling, and grammar corrections (thanks
  Jelle!)
- use RTD as the source for the CHANGELOG too
- visual style cleanups
- add docs about our .gitignore behaviour
- expand GHA Action docs
- clarify we want the PR number in the CHANGELOG entry
- claify Black's behaviour for with statements post Python 3.9
- italicize a bunch of "Black"s

Thank you goes to Jelle, Taneli (hukkinj1 on GH), Felix
(felix-hilden on GH), and Wouter (wbolster on GH) for the feedback!
merge conflicts suck, although these ones weren't too bad.
I consider this important enough to be worthy of a changelog entry :)
@ichard26
Copy link
Collaborator Author

ichard26 commented May 8, 2021

OK, updated the branch and incorporated all of the feedback I've received so far. I know I said I wanted to do these after landing this but given there's going to be no time between this being merged and a release getting pushed, I want these changes to make it in.

@cooperlees I heard you wanted to do a review, so count this as your reminder to so, thank you in advance! Any other final reviews or comments are also welcomed.

Finally a reminder to fellow maintainers that I want to handle merging this myself since I'd like to edit the commit message to my liking.

ambv and others added 2 commits May 8, 2021 11:42
Prettier works fine for all of the default MyST syntax so let's not
rock the boat as much. Dropping the mdformat commit was merge-conflict
filled so here's additional commit instead.
Copy link
Contributor

@hukkin hukkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prettier works fine for all of the default MyST syntax

@ichard26 I can't force you to use mdformat 😄 but will mention that this is not true.

A very simple example doc:

a line
% a comment
another line

Prettier output:

a line % a comment another line

@ichard26
Copy link
Collaborator Author

ichard26 commented May 8, 2021

@hukkinj1 well for that we'll use <! -- {content} --> ... so okay maybe not all of the default syntax is supported but.... for our purposes, it works fine (and while stuff inside roles, directives, etc. aren't formatted by prettier, they're not broken by it). We can always revisit it. My apologies for throwing away your work like that. Thanks for all your help!

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I read closely and found a few things hard to parse so offered suggestions. Take or leave them. Then lets get this sucker merged so we can release awesome docs + refactor ready for PyCon and possible a bug fix or two :D

docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
docs/contributing/gauging_changes.md Outdated Show resolved Hide resolved
docs/contributing/gauging_changes.md Outdated Show resolved Hide resolved
docs/index.rst Outdated
Comment on lines 22 to 26
many projects, small and big. It also sports a decent test suite. However, it is
still very new. Things will probably be wonky for a while. This is made explicit by
the "Beta" trove classifier, as well as by the "b" in the version number. What this
means for you is that **until the formatter becomes stable, you should expect some
formatting to change in the future**. That being said, no drastic stylistic changes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to fussed here, take it of leave it. I just don't like a ~3 year old project being called new.

Suggested change
many projects, small and big. It also sports a decent test suite. However, it is
still very new. Things will probably be wonky for a while. This is made explicit by
the "Beta" trove classifier, as well as by the "b" in the version number. What this
means for you is that **until the formatter becomes stable, you should expect some
formatting to change in the future**. That being said, no drastic stylistic changes
many projects, small and big. Black has a comprehensive test suite, with efficient parallel tests, our own auto formatting and parallel Continuous Integration runner. However, _Black_ is still beta. Things will probably be wonky for a while. This is made explicit by
the "Beta" trove classifier, as well as by the "b" in the version number. What this
means for you is that **until the formatter becomes stable, you should expect some
formatting to change in the future**. That being said, no drastic stylistic changes

docs/integrations/github_actions.md Show resolved Hide resolved
docs/integrations/source_version_control.md Outdated Show resolved Hide resolved
Comment on lines +22 to +32
So _Black_ will eventually format it like this:

```py3
with \
make_context_manager(1) as cm1, \
make_context_manager(2) as cm2, \
make_context_manager(3) as cm3, \
make_context_manager(4) as cm4 \
:
... # backslashes and an ugly stranded colon
```
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this is still true - I get code like this these days: https://github.com/cooperlees/json2netns/blob/main/src/json2netns/tests/netns.py#L43-L45

I guess maybe cause I don't have the ''

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused, the style described in this document is planned, but hasn't be implemented yet. This used to exist the main style doc but that was misleading and confusing.

--version Show the version and exit.
--config FILE Read configuration from FILE path.
-h, --help Show this message and exit.
```
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: I know it's nice, but I hate how these go out of date (And do for other projects). I've wondered if we can in a follow up PR create a CI action to dump --help to a file and include it somehow?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed pypa/bandersnatch#907 for bandersnatch to see if we can POC something there ...

docs/usage_and_configuration/the_basics.md Outdated Show resolved Hide resolved
@hukkin
Copy link
Contributor

hukkin commented May 8, 2021

@hukkinj1 well for that we'll use <! -- {content} --> ... so okay maybe not all of the default syntax is supported but.... for our purposes, it works fine (and while stuff inside roles, directives, etc. aren't formatted by prettier, they're not broken by it). We can always revisit it. My apologies for throwing away your work like that. Thanks for all your help!

No worries. Note that you can disable certain myst syntax in sphinx conf.py. Example here https://github.com/executablebooks/mdformat/blob/ba3385a3e5c08cbf8404b3fb223660047baab2d5/docs/conf.py#L64

Comments, block level math and blockbreaks at least will be broken by prettier, so might be a good idea to disable those syntaxes.

Lots of wording improvements by Cooper. Taneli suggested to disable the
enabled by default MyST syntax not supported by Prettier and I agreed.
And Jelle found one more spelling error!
@ichard26 ichard26 merged commit 62bfbd6 into psf:master May 8, 2021
@ichard26 ichard26 deleted the reorganize-docs-v2 branch May 8, 2021 19:18
@JelleZijlstra
Copy link
Collaborator

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: maintenance Related to project maintenance, e.g. CI, testing, policy changes, releases T: documentation Improvements to the docs (e.g. new topic, correction, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reorganize documentation
7 participants