-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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
Fixed #34140 blacken-docs #16496
Fixed #34140 blacken-docs #16496
Conversation
6796ff3
to
2a2cf4b
Compare
cefd915
to
221ceb7
Compare
P.S. I couldn't get Help on that would be appreciated. I'm referring to this specific issue; I changed several instance of this:
to this:
to get Any help on this, perhaps an obvious solution which I'm missing, would be appreciated! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to write down some suggestions.
docs/ref/databases.txt
Outdated
@@ -577,7 +580,7 @@ Here's a sample configuration which uses a MySQL option file:: | |||
database = NAME | |||
user = USER | |||
password = PASSWORD | |||
default-character-set = utf8 | |||
default - character - set = utf8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this code a different code block from the previous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by marking it setting it to have ini
file pygment:
.. code-block:: ini
docs/ref/databases.txt
Outdated
...: ..., | ||
"NAME": "localhost:1521/orclpdb1", | ||
...: ..., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...: ..., | |
"NAME": "localhost:1521/orclpdb1", | |
...: ..., | |
# ... | |
"NAME": "localhost:1521/orclpdb1", | |
# ... |
As used before maybe can work something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, thanks @pauloxnet !!
docs/ref/databases.txt
Outdated
...: ..., | ||
"NAME": ( | ||
"(DESCRIPTION=(ADDRESS=(PROTOCOL=TCP)(HOST=localhost)(PORT=1521))" | ||
"(CONNECT_DATA=(SERVICE_NAME=orclpdb1)))" | ||
), | ||
...: ..., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...: ..., | |
"NAME": ( | |
"(DESCRIPTION=(ADDRESS=(PROTOCOL=TCP)(HOST=localhost)(PORT=1521))" | |
"(CONNECT_DATA=(SERVICE_NAME=orclpdb1)))" | |
), | |
...: ..., | |
# ... | |
"NAME": ( | |
"(DESCRIPTION=(ADDRESS=(PROTOCOL=TCP)(HOST=localhost)(PORT=1521))" | |
"(CONNECT_DATA=(SERVICE_NAME=orclpdb1)))" | |
), | |
# ... |
See above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pauloxnet - fixed
docs/ref/databases.txt
Outdated
...: ..., | ||
"OPTIONS": { | ||
"threaded": True, | ||
}, | ||
...: ..., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...: ..., | |
"OPTIONS": { | |
"threaded": True, | |
}, | |
...: ..., | |
# ... | |
"OPTIONS": { | |
"threaded": True, | |
}, | |
# ... |
See above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pauloxnet - fixed
docs/ref/databases.txt
Outdated
...: ..., | ||
"OPTIONS": { | ||
"use_returning_into": False, | ||
}, | ||
...: ..., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...: ..., | |
"OPTIONS": { | |
"use_returning_into": False, | |
}, | |
...: ..., | |
# ... | |
"OPTIONS": { | |
"use_returning_into": False, | |
}, | |
# ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pauloxnet - fixed
docs/ref/logging.txt
Outdated
'include_html': True, | ||
'reporter_class': 'somepackage.error_reporter.CustomErrorReporter', | ||
{ | ||
...: ..., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...: ..., | |
# ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pauloxnet - fixed
docs/ref/logging.txt
Outdated
}, | ||
}, | ||
...: ..., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...: ..., | |
# ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pauloxnet - fixed
docs/ref/logging.txt
Outdated
'()': 'django.utils.log.CallbackFilter', | ||
'callback': skip_unreadable_post, | ||
{ | ||
...: ..., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...: ..., | |
# ... |
docs/ref/logging.txt
Outdated
}, | ||
}, | ||
...: ..., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...: ..., | |
# ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pauloxnet - fixed
docs/ref/request-response.txt
Outdated
Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) | ||
|
||
For use in, for example, Django templates, headers can also be looked up | ||
using underscores in place of hyphens:: | ||
|
||
{{ request.headers.user_agent }} | ||
{{request.headers.user_agent}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a python code block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When passing --rst-literal-blocks
to blacken-docs
code blocks are treated as Python code blocks by default. So, should this be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in that code we can use .. code-block:: html+django
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pauloxnet, forgot to apply this, applied now. Rebased and force pushed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is not the complete thing. But it's still time consuming enough to justify creating a PR.
Hey @jvzammit — thanks for pushing this — I know it's fiddly.
Can I suggest you extract the code-block
changes into a single commit.
:mod:`django.apps`:
.. code-block:: pycon
Refs #34140 -- Added missing Sphinx code-block directives for non-Python samples.
That's the most of this (maybe) and it's easy for us to just merge, thereby reducing the scope of the rest of it.
(The other point quickly visible here is that I think making the ...
into explicit comments may be better than the quasi-dict ...: ...
— which is a bit weird.)
docs/ref/databases.txt
Outdated
... | ||
...: ..., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In these cases, is using a comment available:
# ...
Update: I see a0b3803#r1084661525 makes the same point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep changed that, thanks.
The issue is that after changing this I still get these errors on make html
:
docs/ref/logging.txt:20: ERROR: Unexpected indentation.
docs/ref/logging.txt:290: ERROR: Unexpected indentation.
docs/ref/logging.txt:26: WARNING: Definition list ends without a blank line; unexpected unindent.
docs/ref/logging.txt:27: WARNING: Block quote ends without a blank line; unexpected unindent.
docs/ref/logging.txt:29: WARNING: Block quote ends without a blank line; unexpected unindent.
docs/ref/logging.txt:39: ERROR: Unexpected indentation.
docs/ref/logging.txt:309: ERROR: Unexpected indentation.
docs/ref/logging.txt:45: WARNING: Definition list ends without a blank line; unexpected unindent.
docs/ref/logging.txt:46: WARNING: Block quote ends without a blank line; unexpected unindent.
docs/ref/logging.txt:48: WARNING: Block quote ends without a blank line; unexpected unindent.
docs/ref/settings.txt:1696: ERROR: Unexpected indentation.
cc @pauloxnet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, we can look at that 👍
(Let's see if we can get the code-block adjustments merged.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I "fixed" the ref/settings.txt
issue.
But for the logging.txt
ones, I can't spot the solution. But I'm no expert, so it might be an obvious mistake I'm making.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, I'm trying to get the "first" part, i.e. have blacken-docs
work without errors for any of these files.
We can see if it makes sense to have those changes merged. I.e. without the blacken-docs
changes.
That commit would include more than the code-block
changes. But not much more.
If we need a "clean" code-block
changes only commit, I'll create a new PR with that commit only. But first I need to fix the errors (possibly only one error causing in the rest) in ref/logging.txt
👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jvzammit — get it as close as you can, with the energy/time/enthusiasm you have (leaving a buffer! 😅) and I can have a look at any lingerers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it locally and I confirm the same errors/warnings related to docs/ref/logging.txt
but I can't spot the solution either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pauloxnet the indentation changes you suggested fixed the errors 🎉
@carltongibson @pauloxnet I pushed the changes and "a second commit" with the automatic changes by running:
blacken-docs --rst-literal-blocks ref/clickjacking.txt
blacken-docs --rst-literal-blocks ref/databases.txt
blacken-docs --rst-literal-blocks ref/django-admin.txt
blacken-docs --rst-literal-blocks ref/logging.txt
blacken-docs --rst-literal-blocks ref/request-response.txt
blacken-docs --rst-literal-blocks ref/settings.txt
blacken-docs --rst-literal-blocks ref/signals.txt
blacken-docs --rst-literal-blocks ref/template-response.txt
blacken-docs --rst-literal-blocks ref/unicode.txt
blacken-docs --rst-literal-blocks ref/urlresolvers.txt
blacken-docs --rst-literal-blocks ref/utils.txt
I can drop that second commit if it expedites merging the first commit 🚀
After all we can just run the command later on (in a subsequent PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the second commit is ok. I only add one suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pauloxnet I dropped the second commit, apply your correct suggestion, fixup and force push. That puts all "manual" changes prior to running the command in the first commit.
Then ran the command again, and put the changes by the command in the second commit.
How is it looking now?
2d85191
to
4367767
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jvzammit I think I fixed the docs/ref/logging.txt
file errors. Can you try to accept my suggestions to trigger the pipeline?
0d6ac2d
to
3855005
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've found the correct code block for the Django template.
8369eb9
to
ff01267
Compare
LGTM |
@carltongibson I'm afraid of this falling through the cracks. Do we need to do anything else on it? Thanks again! |
Hey @jvzammit — I didn't get the chance to sit down with it yet. (Currently working on tomorrow's releases... 🤹♀️) One query I had last time I looked was whether we could apply the To help me in the shell, what and the steps you take to run & reset? Thanks. |
Thanks for the immediate response @carltongibson !!
That takes precedence, thanks for looking into this.
I'm going through each one-by-one. Why? In some cases it's not
The first commit is all manual. The second commit is the result of running
I know it's far from automated, but it's a good first, small step. With the benefit of actually improving the snippets in current docs. |
2432ead
to
6a53a03
Compare
@felixxm Thank you! It's not done yet, this is only a few files to test the approach. Will continue working on the rest of the files. Should that be part of this PR? Or should I create new PR(s) with the remaining changes? |
I'd do this in a single PR with 3 commits (see #15387):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so this looks much nicer now I think — It's clear exactly what's at stake in blackening bit.
I wonder if you can drum up a team to divide and conquer the docs @jvzammit? (I see lots of keen people 😜) In theory if we can get all the cleanups in place, we can run blacken-docs at a single pass. 🤔 (Likely it's loads more effort than that sounded like writing it 😅)
Thanks @felixxm I like the plan you suggested. I will need help with "add GitHub action & docs changes" step. Can you please provide some pointers on how to achieve that? An issue I have is that I have not found a way to run Running it for a directory raises
@carltongibson I'm "on it" and hope to have something "decent" soon. From what I see after your commit there are a lot let errors to address 🤞 So I will focus on the first commit hoping the other two are straightforward (third one should be fully-automated). Thanks again! |
a6c1084
to
e748340
Compare
e748340
to
0d759c8
Compare
I manually applied all the changes required for The manual changes are in the first commit. The second commit is a "throwaway" commit because it was generated by running import os
import black
from blacken_docs import format_file
DJANGO_DOCS_DIRECTORY = "<repo root>/django/docs"
def get_txt_filename_list(directory, file_extension):
build_dir = directory + "/_build"
releases_dir = directory + "/releases"
paths = []
for root, _, filenames in os.walk(directory):
for filenames in filenames:
if root.startswith(build_dir):
continue # skip build files
if root.startswith(releases_dir):
continue # skip releases files
if filenames.lower().endswith(file_extension.lower()):
paths.append(os.path.join(root, filenames))
return paths
if __name__ == "__main__":
black_mode = black.FileMode()
retv = 0
for filename in get_txt_filename_list(DJANGO_DOCS_DIRECTORY, "txt"):
retv |= format_file(
filename,
black_mode,
skip_errors=True,
rst_literal_blocks=True,
)
print(retv) This leads me to these questions:
I guess that would be "step 2" as @felixxm described above. Right? One more question 👉 with regards to the build error, should I add the "offender words" to
Update: just added the missing words in a separate (the third) commit. Thanks again! |
We could make pre-commit the only way to run linters, as I started in #15178... This would save duplication, and we wouldn't need to set up extra GitHub Actions config, nor would we need to consider a wrapper script or further blacken-docs features to target particular files. |
@adamchainz That's a sound approach. I'll wait for feedback from @carltongibson & @felixxm on how to proceed this. I.e. if your PR is merged soon-ish I could rebase and use that instead of Github actions. |
TBH neither @felixxm nor I are that keen on moving everything into pre-commit. We already have Jenkins, we already have GHA — let's keep using those. (Personally, I also use the tox environments and would be sad to see them go... 🫠) |
I'd add it to the |
@felixxm This works locally:
Downsides of the above:
P.S. Based on @adamchainz 's suggestion here: adamchainz/blacken-docs#224 (comment) |
I'm using: $ find -name "*.txt" -not -path "./_build/*" -not -path "./_theme/*" | xargs blacken-docs --rst-literal-blocks |
@felixxm The
Note that running that will fail for the contents of Are we to exclude the completely? Or should we exclude only versions for, for example, Django 3 and below? Should be doable via regex. |
There are not many errors in release notes so I would fix them. One thing I noticed is |
…or "releases" doc files
…er "releases" directory
a094db6
to
432124a
Compare
@felixxm I pushed changes so that files under P.S. I just changed the ellipsis to cc @pauloxnet -- we're including |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'class': 'logging.FileHandler', | ||
'filename': 'general.log', | ||
'formatter': 'verbose', | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emphasize-lines
should be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixxm Right, thanks for spotting that!
Where should I push changes to? The new PR you opened? Or still this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've already applied edits to #16605.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will merge and backport #16605, then apply blacken-docs
and add a GitHub action as second step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK thanks. What happens to this PR? Should I rebase or close in favour of another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can close it for now, thanks 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @felixxm 👍
Closing as the changes conflict with the backport PR containing the manual changes from this PR: #16605 |
Two commits
Files manually modified in this PR for
blacken-docs --rst-literal-blocks
to run without issue:I know this is not the complete thing. But it's still time consuming enough to justify creating a PR. And get some reviews on some manual decisions I have taken in getting the documentation files able to be handled by
blacken-docs
without errors.For completeness' sake, these are the "previous" errors before commit 1 in this PR: