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

JSONRequest naming issue #3929

Closed
noviluni opened this issue Aug 2, 2019 · 11 comments
Closed

JSONRequest naming issue #3929

noviluni opened this issue Aug 2, 2019 · 11 comments

Comments

@noviluni
Copy link
Member

@noviluni noviluni commented Aug 2, 2019

A few months ago JSONRequest was added to Scrapy (#3505) and released with the new Scrapy version 1.7.

However, I realized that the name is not in accordance with the rest of the classes: HtmlResponse, XmlResponse, XmlRpcRequest.

It should be named:
JsonRequest.

I know it isn't backward-compatible, but as it's a really recent addition and Scrapy 2.0 is on the way it could be fixed.

@noviluni noviluni changed the title JSONResponse naming issue JSONRequest naming issue Aug 2, 2019
@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Aug 5, 2019

Maybe we should consider renaming the other 3 instead to comply with PEP8.

@noviluni
Copy link
Member Author

@noviluni noviluni commented Aug 7, 2019

@Gallaecio Totally agree with you. We should decide what to do:

  • to change it or not

If we decide to change it:

  • to follow PEP8 or not

I have just opened an issue regarding PEP8: #3944. Maybe it should be addressed before deciding the second question.

@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Aug 8, 2019

Hey! As JSONRequest is what pep8 recommends, we (me, @dangra, @Gallaecio and @wRAR) agreed to rename other classes instead. PRs are welcome :)

@kmike at #3930 (comment)

@noviluni
Copy link
Member Author

@noviluni noviluni commented Aug 8, 2019

Looking over I have seen that this issue affects at least the following classes:

HtmlResponse
HtmlParserLinkExtractor

HttpProxyMiddleware
HttpAuthMiddleware
HttpCacheMiddleware
HttpCompressionMiddleware
HttpErrorMiddleware
HttpError

JsonWriterPipeline
JsonLinesItemExporter
JsonItemExporter

XmlRpcRequest
XmlItemExporter
XmlResponse

UrlLengthMiddleware
UrlContract

(Apart of a lot of test classes).

@kmike
Copy link
Member

@kmike kmike commented Aug 8, 2019

👍 Thanks for checking it @noviluni! I was thinking it is less things to fix..

@kmike
Copy link
Member

@kmike kmike commented Aug 8, 2019

Tbh, I'd leave everything as-is, probably including JSONRequest; it is not super consistent, but my feeling is that changing it doesn't worth the development efforts required to upgrade (change in settings, imports, etc.)

@noviluni
Copy link
Member Author

@noviluni noviluni commented Aug 8, 2019

I'm agree with you, there are too much classes to deprecate... If all of you are agree feel free to close this issue.

@kmike
Copy link
Member

@kmike kmike commented Aug 8, 2019

We may still change JSONRequest though, to at least match XmlRpcRequest - that's what you propsed in #3930 .

@noviluni
Copy link
Member Author

@noviluni noviluni commented Aug 9, 2019

As all the classes in Scrapy starting with "JSON" (JsonLinesItemExporter, JsonItemExporter and JsonWriterPipeline) use that format, it could be a good idea.

If you all want to proceed I can re-open the original PR and adding the code to deprecate that class.

@noviluni
Copy link
Member Author

@noviluni noviluni commented Aug 28, 2019

Added new PR: #3982

(I deleted the old branch from my repo and couldn't open the old PR).

@noviluni
Copy link
Member Author

@noviluni noviluni commented Sep 5, 2019

I close this issue as PR has been already merged. Thank you all for your feedback! :)

@noviluni noviluni closed this Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants