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

Response.follow() method not consistent with Request.__init__(). Missing flags. #4277

Closed
LanetheGreat opened this issue Jan 17, 2020 · 4 comments · Fixed by #4279
Closed

Response.follow() method not consistent with Request.__init__(). Missing flags. #4277

LanetheGreat opened this issue Jan 17, 2020 · 4 comments · Fixed by #4279

Comments

@LanetheGreat
Copy link
Contributor

@LanetheGreat LanetheGreat commented Jan 17, 2020

Versions Affected:

All versions after PR #2082 when flags were added to Request. (>1.4.0)

Response.follow() method not consistent with Request.init(). Missing flags.

No sure how this got missed since PR #2082 was merged, but looking at the parameters for Request.__init__ and Response.follow it appears flags wasn't added to Response.follow to keep it in line with how creating new Request instances works. I'm not sure if this was just overlooked or was intentional, when flags support was added to requests. It's also missing for the subclasses of Response as well (TextResponse will need updated, the rest currently use inheritance).

I was looking to be able to add custom flags to certain Response.follow() calls but realized looking at the source they wouldn't be carried over to the new responses, but this can easily be worked around by manually added the flag after Request creation. This seems like an easy fix to include in 1.8.1 or later but doesn't seem too high priority and I just thought you guys should know or that it should at least be noted somewhere on this repo since the docs still say, "It accepts the same arguments as Request.__init__ method...".

@elacuesta
Copy link
Member

@elacuesta elacuesta commented Jan 17, 2020

You're right! Thanks for the report. I think this is a reasonable thing, and a good implementation should not take long to get accepted. Do you plan to submit a PR?

@Prime-5
Copy link
Contributor

@Prime-5 Prime-5 commented Jan 19, 2020

If not done, I would like to submit a PR for this.
As far as I understand, the "flags=None" parameter needs to be added to requests.follow(), right?

@LanetheGreat
Copy link
Contributor Author

@LanetheGreat LanetheGreat commented Jan 19, 2020

Yep, both Response.follow and TextResponse.follow since it looks like the only subclass that overrides that method. Sorry haven't had time to write a PR for it this weekend, been too busy to even touch my laptop.

@Prime-5
Copy link
Contributor

@Prime-5 Prime-5 commented Jan 19, 2020

@LanetheGreat thanks, I'm on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants