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

Sort the list of Request.meta alphabetically #5061 #5065

Merged
merged 5 commits into from
Mar 23, 2021

Conversation

pratik1500
Copy link
Contributor

@pratik1500 pratik1500 commented Mar 23, 2021

Creating the pull request for Sort the list of Request.meta keys alphabetically #5061 sorted the file request-response.rst and arranged them in order.

Resolves #5061

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.

Could you also address this? We need to find where in the documentation users would expect information on how to handle cookies, and mention those two request meta keys to make it easier for users to find them (together) in the documentation.

docs/topics/request-response.rst Outdated Show resolved Hide resolved
docs/topics/request-response.rst Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 23, 2021

Codecov Report

Merging #5065 (89ec012) into master (72e8cea) will increase coverage by 0.37%.
The diff coverage is n/a.

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

@@            Coverage Diff             @@
##           master    #5065      +/-   ##
==========================================
+ Coverage   87.63%   88.01%   +0.37%     
==========================================
  Files         162      162              
  Lines       10301    10301              
  Branches     1501     1501              
==========================================
+ Hits         9027     9066      +39     
+ Misses       1001      965      -36     
+ Partials      273      270       -3     
Impacted Files Coverage Δ
scrapy/crawler.py 88.26% <0.00%> (+1.11%) ⬆️
scrapy/core/downloader/__init__.py 92.48% <0.00%> (+1.50%) ⬆️
scrapy/core/scraper.py 89.57% <0.00%> (+2.45%) ⬆️
scrapy/extensions/feedexport.py 95.32% <0.00%> (+4.31%) ⬆️
scrapy/utils/reactor.py 81.66% <0.00%> (+6.66%) ⬆️
scrapy/utils/test.py 60.93% <0.00%> (+10.93%) ⬆️
scrapy/utils/ssl.py 70.73% <0.00%> (+17.07%) ⬆️
scrapy/utils/asyncgen.py 100.00% <0.00%> (+20.00%) ⬆️

@Gallaecio
Copy link
Member

Please, do not miss #5065 (review)

@Gallaecio
Copy link
Member

Gallaecio commented Mar 23, 2021

Please, see #5065 (review), or more specifically #5061 (comment)

@pratik1500
Copy link
Contributor Author

pratik1500 commented Mar 23, 2021

Sorry for this I am not able to get the #5061 (comment), please would you help me in elaborating in what specific changes need to be done.

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’ve just reviewed existing references, and both meta keys seem discoverable from what I would consider the main entry point for cookies in the documentation, so as far as I’m concern there are no additional changes needed.

@kmike What do you think? Should we make sure we mention them somewhere else?

@pratik1500
Copy link
Contributor Author

Thanks a lot

@kmike
Copy link
Member

kmike commented Mar 23, 2021

Thanks for the fix @pratik1500, and thanks for checking it @Gallaecio!

@kmike kmike merged commit f0e1a33 into scrapy:master Mar 23, 2021
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.

Sort the list of Request.meta keys alphabetically
3 participants