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

implement request_to_curl function #5892

Merged
merged 4 commits into from
Apr 13, 2023
Merged

implement request_to_curl function #5892

merged 4 commits into from
Apr 13, 2023

Conversation

guillermo-bondonno
Copy link
Contributor

Implement request_to_curl util function that generates a curl command string from a scrapy.Request object

scrapy/utils/request.py Outdated Show resolved Hide resolved
@wRAR
Copy link
Member

wRAR commented Apr 11, 2023

Cool!

@codecov
Copy link

codecov bot commented Apr 11, 2023

Codecov Report

Merging #5892 (c6bb8c4) into master (d47c732) will increase coverage by 0.01%.
The diff coverage is 95.45%.

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

@@            Coverage Diff             @@
##           master    #5892      +/-   ##
==========================================
+ Coverage   88.82%   88.83%   +0.01%     
==========================================
  Files         162      162              
  Lines       11108    11129      +21     
  Branches     1805     1811       +6     
==========================================
+ Hits         9867     9887      +20     
  Misses        960      960              
- Partials      281      282       +1     
Impacted Files Coverage Δ
scrapy/utils/request.py 97.61% <93.33%> (-0.58%) ⬇️
scrapy/linkextractors/lxmlhtml.py 95.55% <100.00%> (+0.20%) ⬆️

@wRAR
Copy link
Member

wRAR commented Apr 12, 2023

Can you please also look into pylint failures?

@guillermo-bondonno
Copy link
Contributor Author

Can you please also look into pylint failures?

@wRAR It's about using .format instead of f-strings. I think it's much cleaner to use format in this case instead of doing something like

cookies = f"--cookie '{'; '.join(f'{list(c.keys())[0]}={list(c.values())[0]}' for c in request.cookies)}'"

but if it can't be merged as it is, I'll find a way around it sure.
What do you think?

@wRAR
Copy link
Member

wRAR commented Apr 12, 2023

I think this warning should indeed be disabled, per-line or globally. I'll wait for other opinions though.

@Gallaecio
Copy link
Member

Gallaecio commented Apr 12, 2023

I don’t think format is a significantly better choice here, I would find something like this quite readable:

cookie = "; ".join(f"{k}={v}" for k, v in request.cookies.items())
cookies = f"--cookie '{cookie}'"

No strong opinion, though. I am OK with disabling the check in these lines, but I would keep it globally so that f-strings are at least considered in cases like this one.

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.

I think it would be ideal to cover this somewhere in the docs for discoverability, but at the same time I cannot find a good place in the existing docs for it, so +1 to merge as is.

@Gallaecio Gallaecio merged commit 441ac19 into scrapy:master Apr 13, 2023
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