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

Closes #6340. Deprecate the spider argument to Downloader._get_slot_key() #6352

Merged
merged 2 commits into from
May 10, 2024

Conversation

kumar-sanchay
Copy link
Contributor

Fix #6340

docs/news.rst Outdated
@@ -1262,6 +1262,9 @@ Deprecations
- :func:`scrapy.utils.response.response_httprepr` is now deprecated.
(:issue:`4972`)

- :func:`scrapy.core.downloader.Downloader._get_slot_key` is now deprecated. Consider using its corresponding public method instead.
Copy link
Member

Choose a reason for hiding this comment

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

Can you please wrap the line to 80 chars?

@@ -153,6 +155,14 @@ def _get_slot_key(self, request: Request, spider: Optional[Spider]) -> str:

return key

def _get_slot_key(self, request: Request, spider: Optional[Spider]) -> str:
warnings.warn(
"Use of this protected method is deprecated. Consider using its corresponding public method instead.",
Copy link
Member

Choose a reason for hiding this comment

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

I'd mention the new name explicitly.

Copy link
Member

@wRAR wRAR left a comment

Choose a reason for hiding this comment

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

Looks good with some small changes needed.

@kumar-sanchay
Copy link
Contributor Author

@wRAR done with the changes you suggested

docs/news.rst Outdated
Comment on lines 1265 to 1268
- :func:`scrapy.core.downloader.Downloader._get_slot_key` is now deprecated.
Consider using its corresponding public method get_slot_key() instead.
(:issue:`6340`)

Copy link
Member

Choose a reason for hiding this comment

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

This is a section for an old release. To cover this new change, you should either add it at the very beginning of the file, in a new section for an unreleased version, or do nothing, and hope we don’t forget to cover this when the build the release notes. The first approach is better, but the second approach is the usual and fine as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh ! I see, thank you for pointing this out

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.

It would be nice to have a test that confirms that the old method continues to work as usual but with a warning. But 👍 from me after the release notes issue is handled.

docs/news.rst Outdated

.. _release-2.11.2:

Scrapy 2.11.2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've not added release date here assuming this will be taken care during release

@kumar-sanchay
Copy link
Contributor Author

It would be nice to have a test that confirms that the old method continues to work as usual but with a warning. But 👍 from me after the release notes issue is handled.

Do you believe testing would be beneficial in this case, considering that the deprecated method internally returning new public method ?

@Gallaecio
Copy link
Member

Do you believe testing would be beneficial in this case, considering that the deprecated method internally returning new public method ?

You are right that here it would not add much, I’m OK with not adding it.

@Gallaecio
Copy link
Member

Thank you!

@Gallaecio Gallaecio merged commit c9ef520 into scrapy:master May 10, 2024
25 checks passed
@kumar-sanchay kumar-sanchay deleted the deprecate_method branch May 10, 2024 15:10
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.

Deprecate the spider argument to Downloader._get_slot_key()
3 participants