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

apply black to templates and fixed unit tests Fixes #5809 #5814

Merged
merged 4 commits into from
Feb 2, 2023

Conversation

alexpdev
Copy link
Contributor

@alexpdev alexpdev commented Jan 30, 2023

I ran black formatter on all of the template files in scrapy/templates. There were very few changes that it made, most of which were changing single quotes ' to double quotes ", or adding a single space in between the hash character and everything else for commented out values in the settings.py file.

It did break a few unit tests for the genspider command used regex for string comparisons. I fixed these by making the regex patterns ambiguous to single or double quotes.

Fixes #5809

This shouldn't have any effect on #5808 since I used the same quote ambiguous patterns for the regex in that PR as well.

@wRAR
Copy link
Member

wRAR commented Jan 30, 2023

I think we should also run black on these files in pre-commit?

@alexpdev
Copy link
Contributor Author

alexpdev commented Jan 31, 2023

That I am not sure how to do. When you attempt to run the tool on the .templ files it throws a syntax error because of all the $variable_names that aren't valid python variables. The way I was able to apply it was to use the the scrapy cli to create generic versions, run black on those files and then copied over the changes to the templates.

I suppose I could write up a script would do that whole process automatically?

Or perhaps there is a much easier way that I am not aware of, which is likely?

@Gallaecio
Copy link
Member

I think for templates it might be OK for this to be a one-time thing, given we cannot simply rely on black. It is not only the variables, the commented-out code should be left as it was, it must be distinguishable from regular comments.

@Gallaecio
Copy link
Member

the commented-out code should be left as it was, it must be distinguishable from regular comments.

Could you revert the changes to commented-out code before we merge?

@codecov
Copy link

codecov bot commented Jan 31, 2023

Codecov Report

Merging #5814 (42b0a6c) into master (da15d93) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5814      +/-   ##
==========================================
+ Coverage   88.93%   88.94%   +0.01%     
==========================================
  Files         162      162              
  Lines       10992    11002      +10     
  Branches     1798     1798              
==========================================
+ Hits         9776     9786      +10     
  Misses        937      937              
  Partials      279      279              
Impacted Files Coverage Δ
scrapy/downloadermiddlewares/robotstxt.py 100.00% <0.00%> (ø)
scrapy/pipelines/images.py 97.08% <0.00%> (+0.02%) ⬆️
scrapy/http/request/__init__.py 97.82% <0.00%> (+0.04%) ⬆️
scrapy/pipelines/media.py 98.63% <0.00%> (+0.04%) ⬆️
scrapy/pipelines/files.py 71.42% <0.00%> (+0.09%) ⬆️

scrapy/templates/spiders/csvfeed.tmpl Outdated Show resolved Hide resolved
tests/test_commands.py Show resolved Hide resolved
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.

Awesome!

Comment on lines +41 to +42
# "Accept": "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8",
# "Accept-Language": "en",
Copy link
Member

Choose a reason for hiding this comment

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

Nice indentation catch!

scrapy/templates/spiders/csvfeed.tmpl Outdated Show resolved Hide resolved
@Gallaecio Gallaecio merged commit 8c8894f into scrapy:master Feb 2, 2023
@alexpdev alexpdev deleted the apply_black_to_templates branch February 2, 2023 05:12
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 Python templates
3 participants