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

Add pre-commit hook for 'blacken-docs' #5816

Merged
merged 5 commits into from
Feb 3, 2023
Merged

Add pre-commit hook for 'blacken-docs' #5816

merged 5 commits into from
Feb 3, 2023

Conversation

pankaj1707k
Copy link
Contributor

blacken-docs has been added as a pre-commit hook in .pre-commit-config.yaml, however, it has not been run under this task.

Fixes #5813

@Gallaecio
Copy link
Member

however, it has not been run under this task.

We need to run it as well as part of the task, so that the pre-commit CI job passes. I think running pre-commit --all-files may be all you need.

@codecov
Copy link

codecov bot commented Jan 30, 2023

Codecov Report

Merging #5816 (113fa40) into master (a08d722) will increase coverage by 4.11%.
The diff coverage is n/a.

❗ Current head 113fa40 differs from pull request most recent head 03f32c0. Consider uploading reports for the commit 03f32c0 to get more accurate results

@@            Coverage Diff             @@
##           master    #5816      +/-   ##
==========================================
+ Coverage   84.82%   88.94%   +4.11%     
==========================================
  Files         162      162              
  Lines       10995    11002       +7     
  Branches     1626     1798     +172     
==========================================
+ Hits         9327     9786     +459     
+ Misses       1399      937     -462     
- Partials      269      279      +10     
Impacted Files Coverage Δ
scrapy/downloadermiddlewares/retry.py 100.00% <ø> (ø)
scrapy/logformatter.py 94.59% <ø> (ø)
scrapy/utils/defer.py 97.31% <ø> (+0.03%) ⬆️
scrapy/extension.py 100.00% <0.00%> (ø)
scrapy/utils/url.py 100.00% <0.00%> (ø)
scrapy/commands/edit.py 51.85% <0.00%> (ø)
scrapy/commands/list.py 77.77% <0.00%> (ø)
scrapy/core/spidermw.py 99.44% <0.00%> (ø)
scrapy/responsetypes.py 94.28% <0.00%> (ø)
scrapy/spiders/crawl.py 94.31% <0.00%> (ø)
... and 53 more

@pankaj1707k
Copy link
Contributor Author

Alright, let me try!

@pankaj1707k
Copy link
Contributor Author

@Gallaecio I have run the pre-commit. Please check.

@Gallaecio
Copy link
Member

I see that it did not modify much. I think it is because it does not affect code snippets preceded with ::. It seems that can be addressed using --rst-literal-blocks (https://github.com/adamchainz/blacken-docs#restructuredtext).

Once you do that, please check the output, and see if there is any non-Python code block affected by the change. If so, we could change such blocks to use .. code-block: syntax, or change the Python ones and remove --rst-literal-blocks again, whichever is the easiest.

@pankaj1707k
Copy link
Contributor Author

Running with --rst-literal-blocks formatted all python code. It appears that non-python code was not changed because it failed to parse them as it is visible in the following screenshot. However, I will commit the changes only after verifying this in all concerned files 😅

image

@pankaj1707k
Copy link
Contributor Author

pankaj1707k commented Jan 31, 2023

I have re-run blacken-docs with --rst-literal-blocks and --skip-errors options. Without the --skip-errors option if a non-python code was encountered, the entire file was skipped from scanning.

docs/contributing.rst Outdated Show resolved Hide resolved
@pankaj1707k
Copy link
Contributor Author

pankaj1707k commented Feb 1, 2023

I changed all python snippets to use .. code-block:: python syntax, encountered some syntax errors but fixed them along the way. Do you also want me to use .. code-block:: pycon on python-shell snippets and run the formatter on them as well?

Change python code snippets to begin with '.. code-block:: python' to be recognized by the hook for formatting. All snippets under '::' (rst literal blocks) are ignored.
@Gallaecio
Copy link
Member

Do you also want me to use .. code-block:: pycon on python-shell snippets and run the formatter on them as well?

That would be awesome ❤️

Prepend '.. code-block:: pycon' to make python console blocks detectable by blacken-docs
@pankaj1707k
Copy link
Contributor Author

@Gallaecio I have formatted all python console blocks. Please verify 😅

@pankaj1707k
Copy link
Contributor Author

Moreover, I think it's a good idea to specify somewhere in the documentation guidelines to use .. code-block:: while writing a python snippet. I am not aware if any such guidelines exist currently for writing the docs of this project 😅

@Gallaecio
Copy link
Member

You could extend https://docs.scrapy.org/en/latest/contributing.html#documentation-policies to cover that, but you have already done a lot here. I’ll have a proper look at the new changes as soon as I get a chance. Thanks!

@Gallaecio Gallaecio mentioned this pull request Feb 2, 2023
Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

Amazing work!

docs/intro/overview.rst Outdated Show resolved Hide resolved
docs/intro/tutorial.rst Outdated Show resolved Hide resolved
docs/topics/api.rst Outdated Show resolved Hide resolved
docs/topics/request-response.rst Outdated Show resolved Hide resolved
docs/topics/request-response.rst Outdated Show resolved Hide resolved
docs/topics/selectors.rst Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@pankaj1707k
Copy link
Contributor Author

Amazing work!

Thanks 😅 Will make the suggested changes soon!

@pankaj1707k
Copy link
Contributor Author

Done 👍

@wRAR wRAR merged commit b9f52fe into scrapy:master Feb 3, 2023
@pankaj1707k pankaj1707k deleted the blacken-docs branch February 3, 2023 09:40
@wRAR
Copy link
Member

wRAR commented Feb 4, 2023

Looks like merging this before waiting for tests to finish (which I did accidentally but I thought it was fine) was a mistake, as tests actually don't finish, and the docs tests fail.

I'll create a new issue about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apply Black to docs
3 participants