Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
treq.auth omnibus #315
treq.auth omnibus #315
Changes from 8 commits
a6ffa49
9879f12
ace1faf
1fba8e0
db208fb
16a6d02
6df922a
ade96fa
cc3e26e
e963821
fbabbc1
1a40d0c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good. I think that this test helper would also be very useful in core Twisted HTTP as it might be used by other tests in which twisted.web.iweb.IAgent is used.
If you have time for a review, I could try to add it to Twisted, but I need a review buddy.
If if you want to add it to twisted, I would be happy to review it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be happy to see it added there. Indeed it woudn't surprise me if something similar already exists somewhere in
twisted.web.test
. I'd be happy to review if you port it over, or I can do so.I filed https://twistedmatrix.com/trac/ticket/10072 (with
AgentSpy
as the name) to add this to Twisted. Please self-assign the ticket when you start working on it; I'll do the same if I get to it first.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comment
maybe rename it as
get_http_agent_spy
and allow to pass any list?Right now, having the list managed outside of recorder() makes no difference and maybe just keep it as
get_http_agent_spy
and return a tupple.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the "agent spy" terminology, but I definitely want a function like
recorder()
that is low lines-of-code to use in a test. If using this API takes more than one or two lines of code folks will move it tosetUp
which then makes the tests more verbose and less amenable to local reasoning.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you write one, read many times :) ... and with a good name, you read the name and you don't have to search for the documentation of that name :)
and then, I guess that all developers have auto completion :)
And if someones adds stuff to
setUp
just complain during review.It's your call.
I don't think that is fair to make the code harder to read just because some people are lazy :p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed
recorder
toagent_spy
and_RequestRecordAgent
to_AgentSpy
. The class could be made public to provide the bring-your-own-accumulator API, or another wrapper function added. Probably the latter is best to prevent subclassing.