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

refactor(Response): rename Response.body to Response.text #1764

Merged
merged 14 commits into from
Dec 17, 2020

Conversation

ashutoshvarma
Copy link
Contributor

@ashutoshvarma ashutoshvarma commented Aug 23, 2020

Summary of Changes

This PR renames the Response.body to Response.text in both interfaces. Also Response.body is made deprecated alias for new Response.text

  • All the references are changed in both docs and docstrings
  • getters and setters for Response.body has been decorated with deprecated(is_property=True). I was not sure about deprecation message, so I just wrote what came to my mind first.
  • tests for Response.body are not removed from test_response_media_asgi.py and test_response_media.py instead same test has been added for Response.text

Related Issues

Fix #1578

Pull Request Checklist

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once; it will save you a few review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.

  • Applied changes to both WSGI and ASGI code paths and interfaces (where applicable).
  • Added tests for changed code.
  • Prefixed code comments with GitHub nick and an appropriate prefix.
  • Coding style is consistent with the rest of the framework.
  • Updated documentation for changed code.
    • Added docstrings for any new classes, functions, or modules.
    • Updated docstrings for any modifications to existing code.
    • Updated both WSGI and ASGI docs (where applicable).
    • Added references to new classes, functions, or modules to the relevant RST file under docs/.
    • Updated all relevant supporting documentation files under docs/.
    • A copyright notice is included at the top of any new modules (using your own name or the name of your organization).
    • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
  • Changes (and possible deprecations) have towncrier news fragments under docs/_newsfragments/, with the file name format {issue_number}.{fragment_type}.rst. (Run towncrier --draft to ensure it renders correctly.)

rename Response.body with Response.text.
Response.body is now deprecated alias for Response.text

fix falconry#1578
@codecov
Copy link

codecov bot commented Aug 23, 2020

Codecov Report

Merging #1764 (c0bc7e9) into master (1c5c1b2) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1764   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           54        54           
  Lines         5127      5132    +5     
  Branches       816       816           
=========================================
+ Hits          5127      5132    +5     
Impacted Files Coverage Δ
falcon/asgi/app.py 100.00% <100.00%> (ø)
falcon/asgi/response.py 100.00% <100.00%> (ø)
falcon/http_status.py 100.00% <100.00%> (ø)
falcon/testing/resource.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c5c1b2...c0bc7e9. Read the comment docs.

Copy link
Member

@CaselIT CaselIT 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. There may still be some body mention lingering about though

While le issue did not mention HttpStatus, I think that it should also rename body -> text

Copy link
Member

@kgriffs kgriffs left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

Copy link
Member

@vytas7 vytas7 left a comment

Choose a reason for hiding this comment

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

We ought to also keep a couple of tests referencing the deprecated to property in order to make sure it continues to work as expected.

Furthermore, as you have correctly indicated in the PR checklist, Towcrier fragment(s) are still missing for the change.

@vytas7
Copy link
Member

vytas7 commented Sep 20, 2020

@ashutoshvarma just checking if you might have a chance to continue working on this (by addressing @kgriffs and @CaselIT comments) within the next couple of weeks?

@ashutoshvarma
Copy link
Contributor Author

Sorry it took me so long to respond, I had been busy with stuff.
Yeah I will try to work on changes suggested by @CaselIT and @kgriffs by this week.

@vytas7
Copy link
Member

vytas7 commented Sep 20, 2020

No worries @ashutoshvarma, and thanks for the update.

@vytas7
Copy link
Member

vytas7 commented Nov 24, 2020

Hi @ashutoshvarma !
Sorry for persistence, but just checking again... the same question 😈

@CaselIT
Copy link
Member

CaselIT commented Nov 24, 2020

Otherwise if you are ok with it, we could take this to the finish line :)

@ashutoshvarma
Copy link
Contributor Author

Terribly sorry for not fulfilling my commitment.
You can go ahead with it, I don't think I will be able to complete this.

@vytas7
Copy link
Member

vytas7 commented Nov 26, 2020

Heh, no worries @ashutoshvarma !
As a volunteer devoting free time to help an open source project, you have no commitments to us!

@CaselIT would you like to take this over the finish line?

@vytas7 vytas7 added the needs contributor Comment on this issue if you'd like to volunteer to work on this. Thanks! label Nov 26, 2020
@vytas7
Copy link
Member

vytas7 commented Nov 29, 2020

Is someone still working on this? I'm new to contributing so I thought this would be interesting to tackle if it isn't still taken.

(As I understand this was a limited Black Weekend offer that has now expired 😈 .)

@maknop Yes, you are very welcome to take over if you still wish to!

@CaselIT
Copy link
Member

CaselIT commented Nov 30, 2020

Heh, no worries @ashutoshvarma !
As a volunteer devoting free time to help an open source project, you have no commitments to us!

@CaselIT would you like to take this over the finish line?

I may be able to work a bit on it this week

@CaselIT CaselIT requested review from kgriffs and vytas7 December 12, 2020 14:35
@CaselIT
Copy link
Member

CaselIT commented Dec 12, 2020

A bit late, but I've managed to come around and finish this

@CaselIT CaselIT removed the needs contributor Comment on this issue if you'd like to volunteer to work on this. Thanks! label Dec 12, 2020
CaselIT
CaselIT previously approved these changes Dec 15, 2020
Copy link
Member

@vytas7 vytas7 left a comment

Choose a reason for hiding this comment

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

This is looking good 👍

I could, however, grep a couple of items that could be revised too:

  • docs/_newsfragments/1679.breakingchange.rst
  • docs/user/recipes/output-csv.rst

@vytas7 vytas7 changed the title rename Response.body to Response.text for both the WSGI and ASGI interfaces refactor(Response): rename Response.body to Response.text Dec 16, 2020
# Conflicts:
#	falcon/app.py
#	falcon/app_helpers.py
#	tests/test_httperror.py
#	tests/test_middleware.py
Copy link
Member

@kgriffs kgriffs 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!

@vytas7 vytas7 merged commit ce29af7 into falconry:master Dec 17, 2020
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.

Rename resp.body to resp.text
4 participants