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

Deprecate body as unicode #4555

Merged
merged 10 commits into from May 11, 2020
Merged

Conversation

nsirletti
Copy link
Contributor

@nsirletti nsirletti commented May 8, 2020

Fixes #4546

My first PR on this project so advice/corrections are very welcome.

Copy link
Member

@Gallaecio Gallaecio left a comment

The previous tests should remain there, as the method is meant to continue to work for some time, simply logging a warning in addition to its usual behavior.

If tests fail, it may be because @deprecated is intended for module-level functions, not methods. Simply using warnings.warn within the method should be a good alternative, though. And the warning should mention that the text attribute should be used instead.

docs/topics/request-response.rst Outdated Show resolved Hide resolved
scrapy/http/response/text.py Outdated Show resolved Hide resolved
Co-authored-by: Adrián Chaves <adrian@chaves.io>
@codecov
Copy link

codecov bot commented May 8, 2020

Codecov Report

Merging #4555 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4555      +/-   ##
==========================================
+ Coverage   84.51%   84.53%   +0.02%     
==========================================
  Files         164      164              
  Lines        9904     9911       +7     
  Branches     1482     1475       -7     
==========================================
+ Hits         8370     8378       +8     
  Misses       1266     1266              
+ Partials      268      267       -1     
Impacted Files Coverage Δ
scrapy/http/response/text.py 100.00% <100.00%> (ø)
scrapy/utils/trackref.py 82.85% <0.00%> (-2.86%) ⬇️
scrapy/pipelines/files.py 59.93% <0.00%> (ø)
scrapy/linkextractors/lxmlhtml.py 95.00% <0.00%> (+2.89%) ⬆️

@nsirletti
Copy link
Contributor Author

nsirletti commented May 8, 2020

Thanks a lot for the review ! I'm on it.

Now this might be a weird question but all this is very new to me. From this point should git revert the test removal and create a new commit, right ? As opposed to rebasing to get a cleaner log.

@elacuesta
Copy link
Member

elacuesta commented May 8, 2020

Thanks for the PR @nsirletti! Don't worry about the commit history, we can always squash when merging.
Regarding tests, please remember to add a check to make sure the deprecation warning is actually logged. You could find a similar case here.

scrapy/http/response/text.py Outdated Show resolved Hide resolved
scrapy/http/response/text.py Outdated Show resolved Hide resolved
Copy link
Member

@Gallaecio Gallaecio left a comment

Beyond the nitpicks, it looks ✔️ to me

@Gallaecio Gallaecio merged commit cb8140a into scrapy:master May 11, 2020
2 checks passed
@Gallaecio
Copy link
Member

Gallaecio commented May 11, 2020

Thanks!

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