Skip to content

Conversation

@AngelikiBoura
Copy link
Contributor

Changes:

  • Add a second regular expression pattern with different ordering in meta tags
  • Test that redirection occurs when the attributes of the meta tag are differently ordered

AngelikiBoura and others added 3 commits June 13, 2022 23:25
Changes:

- Add a second regex pattern with different ordering in meta tags
- Test that redirection occurs when dtags are differently ordered
Remove unnecessary comments and repeated code.
For issue scrapy#162 Add different regex pattern to search for meta tags
@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

Merging #179 (e594385) into master (d9763db) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head e594385 differs from pull request most recent head a0a0b0d. Consider uploading reports for the commit a0a0b0d to get more accurate results

@@           Coverage Diff           @@
##           master     #179   +/-   ##
=======================================
  Coverage   95.68%   95.68%           
=======================================
  Files           7        7           
  Lines         463      464    +1     
  Branches       88       89    +1     
=======================================
+ Hits          443      444    +1     
  Misses         10       10           
  Partials       10       10           
Impacted Files Coverage Δ
w3lib/html.py 95.49% <100.00%> (+0.04%) ⬆️
w3lib/url.py 98.02% <100.00%> (ø)

So the new pattern has the same flexibility with
the original regex pattern
@Gallaecio
Copy link
Member

Can you run black on the code? It seems even something in the main branch fails the black check now, we probably need to freeze the version of black that we use.

@AngelikiBoura
Copy link
Contributor Author

I ran it with tox and this is the result:

angeliki@angeliki-B550M-DS3H:~/lab/library/w3lib$ tox -e black
GLOB sdist-make: /home/angeliki/lab/library/w3lib/setup.py
black inst-nodeps: /home/angeliki/lab/library/w3lib/.tox/.tmp/package/1/w3lib-1.22.0.zip
black installed: black==22.3.0,click==8.1.3,mypy-extensions==0.4.3,pathspec==0.9.0,platformdirs==2.5.2,tomli==2.0.1,typing-extensions==4.2.0,w3lib==1.22.0
black run-test-pre: PYTHONHASHSEED='1496754552'
black run-test: commands[0] | black --check conftest.py setup.py tests w3lib
would reformat w3lib/html.py
would reformat w3lib/url.py
would reformat tests/test_html.py

Oh no! 💥 💔 💥
3 files would be reformatted, 12 files would be left unchanged.
ERROR: InvocationError for command /home/angeliki/lab/library/w3lib/.tox/black/bin/black --check conftest.py setup.py tests w3lib (exited with code 1)
___________________________________ summary ____________________________________
ERROR: black: commands failed

@Gallaecio
Copy link
Member

Instead of using tox, which just checks the files, if you run black conftest.py setup.py tests w3lib (the command that tox runs, without --check) you can actually modify the required files so that tox -e black passes.

@AngelikiBoura
Copy link
Contributor Author

I ran black conftest.py setup.py tests w3lib but it only changed tests/test_html.py and not the other two. So tox -e black still fails.

AngelikiBoura and others added 3 commits June 15, 2022 17:20
In test_html the reformat was done by black
@Gallaecio
Copy link
Member

Gallaecio commented Jun 15, 2022

You were probably using an older black. I have run the latest black, and frozen black in tox.ini so that this will not happen again (that a newer black causes code already in the main branch to stop passing CI).

@Gallaecio
Copy link
Member

Addressed. Sorry for the unrelated noise, I was not very careful when I added those static checks 🤦

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.

Thanks!

@AngelikiBoura
Copy link
Contributor Author

No problem! Thank you, too.

@kmike
Copy link
Member

kmike commented Jun 16, 2022

Thanks @AngelikiBoura!

@kmike kmike merged commit e94e699 into scrapy:master Jun 16, 2022
@kmike kmike added this to the 2.0.0 milestone Aug 8, 2022
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.

3 participants