build: 'check_format.py check' reports line number#2390
Merged
mattklein123 merged 6 commits intoenvoyproxy:masterfrom Jan 18, 2018
Merged
build: 'check_format.py check' reports line number#2390mattklein123 merged 6 commits intoenvoyproxy:masterfrom
mattklein123 merged 6 commits intoenvoyproxy:masterfrom
Conversation
To quickly find issues. Example outputs: $ ./tools/check_format.py check source/common/stats/statsd.cc ERROR: header_order.py check failed for file: ./source/common/stats/statsd.cc ERROR: ./source/common/stats/statsd.cc:3 ERROR: ./source/common/stats/statsd.cc:9 ERROR: ./source/common/stats/statsd.cc:12 ERROR: clang-format check failed for file: ./source/common/stats/statsd.cc ERROR: ./source/common/stats/statsd.cc:26 ERROR: ./source/common/stats/statsd.cc:38 ERROR: ./source/common/stats/statsd.cc:46 ERROR: check format failed. run 'tools/check_format.py fix' $ ./tools/check_format.py check source/common/stats/statsd.h ERROR: ./source/common/stats/statsd.h has over-enthusiastic spaces: ERROR: ./source/common/stats/statsd.h:128 ERROR: header_order.py check failed for file: ./source/common/stats/statsd.h ERROR: ./source/common/stats/statsd.h:7 ERROR: ./source/common/stats/statsd.h:12 ERROR: clang-format check failed for file: ./source/common/stats/statsd.h ERROR: ./source/common/stats/statsd.h:29 ERROR: ./source/common/stats/statsd.h:128 ERROR: ./source/common/stats/statsd.h:146 ERROR: check format failed. run 'tools/check_format.py fix' $ ./tools/check_format.py check source/common/stats/BUILD ERROR: buildifier check failed for file: ./source/common/stats/BUILD ERROR: ./source/common/stats/BUILD:16 ERROR: ./source/common/stats/BUILD:24 ERROR: ./source/common/stats/BUILD has unexpected direct external dependency on protobuf, use //source/common/protobuf instead. ERROR: ./source/common/stats/BUILD:24 ERROR: check format failed. run 'tools/check_format.py fix' Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Member
|
Thank you @taiki45! @danielhochman please review. |
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
tools/check_format.py
Outdated
| with open(file_path) as f: | ||
| if re.search('"protobuf"', f.read(), re.MULTILINE): | ||
| text = f.read() | ||
| if re.search(pattern, text, re.MULTILINE): |
Contributor
There was a problem hiding this comment.
why use re? why not use in?
Contributor
There was a problem hiding this comment.
ah i see why, see other comment
tools/check_format.py
Outdated
| if re.search(pattern, text, re.MULTILINE): | ||
| printError("%s has over-enthusiastic spaces:" % file_path) | ||
| regex = re.compile(pattern) | ||
| for i, line in enumerate(text.splitlines()): |
Contributor
There was a problem hiding this comment.
i see why re is useful here. can we abstract the find and output line number logic into a function to use it in both places?
Member
Author
There was a problem hiding this comment.
Sorry for the confusion but I also think we can use in instead of re.search. Is re.search still useful here?
Don't use costly regex search. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
tools/check_format.py
Outdated
| print "ERROR: something went wrong while executing: %s" % e.cmd | ||
| sys.exit(1) | ||
| # In case we can't find any line numbers, call printError at first. | ||
| printError(error_message + " for file: %s" % (file_path)) |
Contributor
There was a problem hiding this comment.
consistent string formatting please
i.e. "%s for file: %s" % (error_message, file_path)
tools/check_format.py
Outdated
| def checkProtobufExternalDepsBuild(file_path): | ||
| if whitelistedForProtobufDeps(file_path): | ||
| return True | ||
| def findPatternAndPrintError(pattern, file_path, error_message): |
Contributor
There was a problem hiding this comment.
i would rename from pattern (since it's no longer re-based) to substring
Contributor
|
+1 pending minor nits. thanks! |
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
| text = f.read() | ||
| if re.search('"google/protobuf', text, re.MULTILINE) or re.search( | ||
| "google::protobuf", text, re.MULTILINE): | ||
| if '"google/protobuf' in text or "google::protobuf" in text: |
Contributor
There was a problem hiding this comment.
is the leading quote intentional?
Member
Author
There was a problem hiding this comment.
I think this is intentional to detect "include" statements like https://github.com/envoyproxy/envoy/pull/1236/files#diff-c0a36114a7b25e540b0cef252d0993e3L10, since this pattern is introduced with #1236.
danielhochman
approved these changes
Jan 18, 2018
mattklein123
approved these changes
Jan 18, 2018
jpsim
pushed a commit
that referenced
this pull request
Nov 28, 2022
Description: Make the lookup of headers in HeadersBuilder and Headers case-insensitive and preserve the original casing of headers. Implemented on iOS only. Android side-changes are going to be implemented in a follow up PR. GH issue: #2390. Risk Level: Low Testing: Unit Tests Docs Changes: Updated Release Notes: Updated Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>
jpsim
pushed a commit
that referenced
this pull request
Nov 29, 2022
Description: Make the lookup of headers in HeadersBuilder and Headers case-insensitive and preserve the original casing of headers. Implemented on iOS only. Android side-changes are going to be implemented in a follow up PR. GH issue: #2390. Risk Level: Low Testing: Unit Tests Docs Changes: Updated Release Notes: Updated Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Description:
Fixes #2298.
Example outputs:
Risk Level:
Low: Small bug fix or small optional feature.
Testing:
Manual testing in both macOS and Linux (Ubuntu 16.04.3 LTS).
Docs Changes:
N/A
Release Notes:
N/A
Fixes #2298.