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

Fix JSONRequest naming #3982

Merged
merged 3 commits into from Aug 30, 2019
Merged

Fix JSONRequest naming #3982

merged 3 commits into from Aug 30, 2019

Conversation

noviluni
Copy link
Member

@noviluni noviluni commented Aug 28, 2019

Change JSONRequest to JsonRequest to follow the same format/style as the other JSON classes (JsonLinesItemExporter, JsonItemExporter and JsonWriterPipeline) and the other response/request classes (HtmlResponse, XmlResponse, XmlRpcRequest)

fixes:

@codecov
Copy link

@codecov codecov bot commented Aug 28, 2019

Codecov Report

Merging #3982 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3982      +/-   ##
==========================================
+ Coverage   85.39%   85.39%   +<.01%     
==========================================
  Files         167      167              
  Lines        9720     9726       +6     
  Branches     1455     1456       +1     
==========================================
+ Hits         8300     8306       +6     
  Misses       1162     1162              
  Partials      258      258
Impacted Files Coverage Δ
scrapy/http/__init__.py 100% <100%> (ø) ⬆️
scrapy/http/request/json_request.py 94.11% <100%> (+0.36%) ⬆️
scrapy/exporters.py 100% <0%> (ø) ⬆️
scrapy/downloadermiddlewares/robotstxt.py 100% <0%> (ø) ⬆️
scrapy/settings/default_settings.py 98.7% <0%> (ø) ⬆️

kmike
kmike approved these changes Aug 28, 2019
Copy link
Member

@kmike kmike left a comment

Ok, I'm fine with these changes now :)

@@ -64,7 +64,7 @@ New features
provides a cleaner way to pass keyword arguments to callback methods
(:issue:`1138`, :issue:`3563`)

* A new :class:`~scrapy.http.JSONRequest` class offers a more convenient way
Copy link
Member

@elacuesta elacuesta Aug 28, 2019

Choose a reason for hiding this comment

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

I think this line should be left unchanged, since the class was indeed called JSONRequest when it was introduced in 1.7. The renaming/deprecation should be mentioned in the 1.8 release notes.

docs/news.rst Outdated
@@ -64,7 +64,7 @@ New features
provides a cleaner way to pass keyword arguments to callback methods
(:issue:`1138`, :issue:`3563`)

* A new :class:`~scrapy.http.JSONRequest` class offers a more convenient way
* A new :class:`~scrapy.http.JsonRequest` class offers a more convenient way
Copy link
Member

@Gallaecio Gallaecio Aug 29, 2019

Choose a reason for hiding this comment

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

To comply with @elacuesta’s feedback while keeping a working link:

Suggested change
* A new :class:`~scrapy.http.JsonRequest` class offers a more convenient way
* A new :class:`JSONRequest <scrapy.http.JsonRequest>` class offers a more convenient way

Copy link
Member Author

@noviluni noviluni Aug 29, 2019

Choose a reason for hiding this comment

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

Tested in local and it works, thanks 🙂

Copy link
Member

@elacuesta elacuesta Aug 29, 2019

Choose a reason for hiding this comment

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

Nice, I forgot this generated a link.

@noviluni
Copy link
Member Author

@noviluni noviluni commented Aug 29, 2019

Release notes fixed

@Gallaecio Gallaecio merged commit ace2df3 into scrapy:master Aug 30, 2019
2 checks passed
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.

None yet

4 participants