code_format: fix path matching#20108
Conversation
This change fixes an issue in the code formatter where explicitly excluded paths are incorrectly matched when the script is provided arguments that do not start with './' or that end with a trailing slash. To address this issue this change normalizes paths before checking if the path is excluded. Signed-off-by: David Peet <davidpeet@tutanota.com>
|
Hi @DavidPeet8, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
|
Thanks for your contribution! Please wait for a maintainer to trigger CI flow. /cc @envoyproxy/stable-maintainers |
| def normalize_path(path): | ||
| """Convert path to form ./path/to/dir/ for directories and ./path/to/file otherwise""" | ||
| isdir = os.path.isdir(args.target_path) | ||
| if not path.startswith("./"): |
There was a problem hiding this comment.
Would it make sense to prepend "./" before checking if it is a directory? Just in case.
There was a problem hiding this comment.
im wondering if pathlib could be helpful here - it does quite a bit of path normalization - altho it seems like elsewhere is interpreting the appended / so maybe that just complicates things
There was a problem hiding this comment.
Would it make sense to prepend "./" before checking if it is a directory? Just in case.
@yanavlasov - Good point, calling this with an absolute path would lead to unexpected behaviour
im wondering if
pathlibcould be helpful here - it does quite a bit of path normalization - altho it seems like elsewhere is interpreting the appended/so maybe that just complicates things
@phlax - I think in the future something like pathlib could be quite helpful, but I think the introduction of path normalization via pathlib or some other library function should come with a larger refactor in this file. In particular, I think a diff changing to normalize via a library would be a good opportunity to refactor how paths are handled in general across the formatter.
As you mentioned, the trailing slash becomes a bit awkward to deal with as we would be modifying the path after it is normalized - hopefully this could be solved with such an additional refactor
Signed-off-by: David Peet <davidpeet@tutanota.com>
|
@phlax - Are you able to trigger the pending workflow? |
phlax
left a comment
phlax
left a comment
There was a problem hiding this comment.
lgtm, thanks @DavidPeet8 for working through this
- no changes in `.bazelrc`, `.bazelversion`, `ci/run_envoy_docker.sh` or `tools/gen_compilation_database.py`. - introducing a workaround for our use of the Envoy's `check_format.py` after envoyproxy/envoy#20108. See #815 for details. Signed-off-by: Jakub Sobon <mumak@google.com>
4 participants
Commit Message: This change fixes an issue in the code formatter where explicitly
excluded paths are incorrectly matched when the script is provided
arguments that do not start with './' or that end with a trailing
slash.
To address this issue this change normalizes paths before checking
if the path is excluded.
Additional Description: I think the core issue here is the use of direct string matching for excluded files. Refactoring to address the root cause is out of scope for this change, but will likely need to be done in the future.
Risk Level: low
Testing: Ran code format with arguments of the form 'path/to/dir' and './path/to/dir/', verifying that excluded files are not formatted as expected.
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
Fixes: #20077