Skip to content

gRPC Python Client and Server Interceptors#13722

Merged
mehrdada merged 4 commits intogrpc:v1.8.xfrom
mehrdada:ship-py-interceptors
Dec 12, 2017
Merged

gRPC Python Client and Server Interceptors#13722
mehrdada merged 4 commits intogrpc:v1.8.xfrom
mehrdada:ship-py-interceptors

Conversation

@mehrdada
Copy link
Copy Markdown
Contributor

@mehrdada mehrdada commented Dec 12, 2017

No description provided.

@mehrdada mehrdada mentioned this pull request Dec 12, 2017
Closed
@mehrdada
Copy link
Copy Markdown
Contributor Author

mehrdada commented Dec 12, 2017

@nathanielmanistaatgoogle PTAL (I plan to yapf the broader examples/python directory afterward, so please don't worry about that.)

nathanielmanistaatgoogle
nathanielmanistaatgoogle suggested changes Dec 12, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@nathanielmanistaatgoogle nathanielmanistaatgoogle left a comment

Choose a reason for hiding this comment

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

Looks mostly good! Most of my comments are minor.

examples/python/interceptors/default_value/.gitignore Outdated
Copy link
Copy Markdown
Contributor

@nathanielmanistaatgoogle nathanielmanistaatgoogle Dec 12, 2017

Choose a reason for hiding this comment

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

Why is this necessary? Doesn't the repository top-level .gitignore file also affect this subdirectory?

examples/python/interceptors/default_value/greeter_client.py Outdated
Copy link
Copy Markdown
Contributor

@nathanielmanistaatgoogle nathanielmanistaatgoogle Dec 12, 2017

Choose a reason for hiding this comment

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

Import only packages and modules (almost always modules!); never classes, functions, or other elements of modules.

examples/python/interceptors/default_value/greeter_client.py Outdated
Copy link
Copy Markdown
Contributor

@nathanielmanistaatgoogle nathanielmanistaatgoogle Dec 12, 2017

Choose a reason for hiding this comment

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

The name gRPC is always styled "gRPC".

examples/python/interceptors/default_value/greeter_client.py Outdated
Copy link
Copy Markdown
Contributor

@nathanielmanistaatgoogle nathanielmanistaatgoogle Dec 12, 2017

Choose a reason for hiding this comment

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

Incorrect date.

examples/python/interceptors/default_value/greeter_server.py Outdated
Copy link
Copy Markdown
Contributor

@nathanielmanistaatgoogle nathanielmanistaatgoogle Dec 12, 2017

Choose a reason for hiding this comment

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

Stale date.

examples/python/interceptors/headers/generic_client_interceptor.py Outdated
Copy link
Copy Markdown
Contributor

@nathanielmanistaatgoogle nathanielmanistaatgoogle Dec 12, 2017

Choose a reason for hiding this comment

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

Tuck this into an else: (assumming "return <conditional expression>" doesn't fit on a single line).

examples/python/interceptors/headers/greeter_client.py Outdated
Copy link
Copy Markdown
Contributor

@nathanielmanistaatgoogle nathanielmanistaatgoogle Dec 12, 2017

Choose a reason for hiding this comment

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

Stale date.

examples/python/interceptors/headers/greeter_server.py Outdated
Copy link
Copy Markdown
Contributor

@nathanielmanistaatgoogle nathanielmanistaatgoogle Dec 12, 2017

Choose a reason for hiding this comment

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

Stale date.

src/python/grpcio/grpc/_interceptor.py Outdated
Copy link
Copy Markdown
Contributor

@nathanielmanistaatgoogle nathanielmanistaatgoogle Dec 12, 2017

Choose a reason for hiding this comment

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

return <conditional expression> since it fits on a single line.

src/python/grpcio/grpc/_server.py Outdated
Copy link
Copy Markdown
Contributor

@nathanielmanistaatgoogle nathanielmanistaatgoogle Dec 12, 2017

Choose a reason for hiding this comment

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

Tuck this into an else:.

@ctiller
Copy link
Copy Markdown
Member

ctiller commented Dec 12, 2017

Are we really putting major new functionality directly into a release branch and marking it P0?

@mehrdada
Copy link
Copy Markdown
Contributor Author

mehrdada commented Dec 12, 2017 •
edited
Loading

@ctiller There was a prior PR on tested on master substantially similar to this. We just have users who need it in 1.8, so this is a new PR targeting v1.8.x. The delta to the core functionality is minimal.

There was no other label to ensure we don't cut the release accidentally before merging this (i.e. "RELEASE BLOCKER" without implied "p0")

@mehrdada
Copy link
Copy Markdown
Contributor Author

mehrdada commented Dec 12, 2017

@nathanielmanistaatgoogle concerns addressed. PTAL.

@mehrdada mehrdada merged commit a258a49 into grpc:v1.8.x Dec 12, 2017
@mehrdada mehrdada deleted the ship-py-interceptors branch December 12, 2017 21:08
@mehrdada mehrdada mentioned this pull request Dec 12, 2017
Closed
@rnburn rnburn mentioned this pull request Feb 2, 2018
Closed
@lidizheng lidizheng mentioned this pull request Jan 18, 2019
Merged
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@nathanielmanistaatgoogle nathanielmanistaatgoogle nathanielmanistaatgoogle approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@nathanielmanistaatgoogle nathanielmanistaatgoogle

Labels

area/api kind/enhancement lang/Python priority/P0/RELEASE BLOCKER

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mehrdada @ctiller @nathanielmanistaatgoogle @jtattermusch