-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
format: upgrade to clang-format-5.0 #1212
Conversation
@lyft/network-team @htuch |
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.
LGTM
Http::CodeUtility::ResponseStatInfo info{config_->scope(), cluster_->statsScope(), EMPTY_STRING, | ||
*getTooManyRequestsHeader(), true, EMPTY_STRING, | ||
EMPTY_STRING, EMPTY_STRING, EMPTY_STRING, false}; | ||
Http::CodeUtility::ResponseStatInfo info{config_->scope(), |
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.
Do we want this style of overflow for initializers? Seems it would make sense to have them match function argument style, where I think we are packing as much as we can into the line.
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.
Yes I agree. Let me play with it a bit. There is something broken with header ordering also. I think I need to disable the header ordering feature. Let me play with it and I will tag you again.
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.
Best to disable the header ordering in clang-format
. I recall looking into whether we could get it to do what we wanted and concluded no. See #793. I think it had to do with the block splitting etc. Might be better in 5.0, not sure.
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.
Yeah I'm trying to figure out how to disable it in clang-format
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.
@htuch AFAICT all bin packing options are enabled. I'm not sure this is configurable. I will keep poking around but do you know of which option I should be looking at?
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 looked around and I can't find an obvious way to fix this, unless we change AlignAfterOpenBracket to DontAlign which changes a whole bunch of other things. Will leave for now and we can see if we can change this later. I agree it would be nice if it were like a normal function call.
Automatic merge from submit-queue. Add the allow_missing_or_failed option to jwt-auth **What this PR does / why we need it**: Add the allow_missing_or_failed option to jwt-auth so jwt-auth won't return 401 when jwt is missing or the jwt authentication fails. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes envoyproxy#1212 **Special notes for your reviewer**: **Release note**: ```release-note ```
…sentation (#1212) Description: Splits list-formatted header value on creation of header maps. Per RFC 7230, does not perform this parsing on "set-cookie" headers, and additionally skips it on "www-authenticate" and "proxy-authenticate", due to their potential to contain commas in contravention of the spec. Risk Level: Low Testing: Pending Signed-off-by: Mike Schore <[email protected]> Signed-off-by: JP Simard <[email protected]>
…dd specific logic to the RetryPolicy classes (#1752) Description: reverts earlier logic that splits comma-separated headers introduced in #1212 #1213. Upon further inspection of the Envoy code we now understand that the Envoy behavior to coalesce headers with multiple appearances in the header map is limited to Envoy's inline headers. Thus this PR reverts the Envoy Mobile logic to split up headers which should have no semantic impact in headers per RFC. However, we do add logic to the Envoy Mobile RetryPolicy classes which need individual values in order to correctly map to the swift/kotlin enum cases specific to those classes. Risk Level: med - changes header handling logic Testing: added unit tests Signed-off-by: Jose Nino <[email protected]> Signed-off-by: JP Simard <[email protected]>
…sentation (#1212) Description: Splits list-formatted header value on creation of header maps. Per RFC 7230, does not perform this parsing on "set-cookie" headers, and additionally skips it on "www-authenticate" and "proxy-authenticate", due to their potential to contain commas in contravention of the spec. Risk Level: Low Testing: Pending Signed-off-by: Mike Schore <[email protected]> Signed-off-by: JP Simard <[email protected]>
…dd specific logic to the RetryPolicy classes (#1752) Description: reverts earlier logic that splits comma-separated headers introduced in #1212 #1213. Upon further inspection of the Envoy code we now understand that the Envoy behavior to coalesce headers with multiple appearances in the header map is limited to Envoy's inline headers. Thus this PR reverts the Envoy Mobile logic to split up headers which should have no semantic impact in headers per RFC. However, we do add logic to the Envoy Mobile RetryPolicy classes which need individual values in order to correctly map to the swift/kotlin enum cases specific to those classes. Risk Level: med - changes header handling logic Testing: added unit tests Signed-off-by: Jose Nino <[email protected]> Signed-off-by: JP Simard <[email protected]>
This most enforces namespace comments but also churns/fixes a few
other things.
Fixes #1200
Fixes #673