Skip to content
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

Make the fix_format and check_format scripts work on Nighthawk code #916

Merged
merged 3 commits into from
Sep 23, 2022

Conversation

dubious90
Copy link
Contributor

@dubious90 dubious90 commented Sep 21, 2022

Fixes #913

Direct changes:

Also ran the format script to fix all current code formatting errors.

Signed-off-by: Nathan Perry [email protected]

Nathan Perry added 2 commits September 21, 2022 17:01
Also run the format script to fix all current code formatting errors.

Signed-off-by: Nathan Perry <[email protected]>
Signed-off-by: Nathan Perry <[email protected]>
@dubious90
Copy link
Contributor Author

@qqustc Please review and assign to @mum4k when done.

@dubious90 dubious90 added the waiting-for-review A PR waiting for a review. label Sep 21, 2022
qqustc
qqustc previously approved these changes Sep 22, 2022
tools/code_format/config.yaml Outdated Show resolved Hide resolved
tools/check_format.sh Show resolved Hide resolved
- envoy # unique
- nighthawk # unique
- external/source/envoy # unique
- external # unique
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am trying to verify if this list contains all the directories it should, but I am having a hard time figuring out how these map into the list of our actual directories here:

https://github.com/envoyproxy/nighthawk

This seems to be a list of top level and sub-level directories. Can you help me understand what this list should contain and how it is used?

E.g. the following directories are missing here, is that intentional?

  • benchmarks (no c++ code, so probably ok)
  • adaptive_load

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The short answer is I'm not 100% sure what this list's intention is. I copied this directly from:
--include_dir_order envoy,nighthawk,external/source/envoy,external,api,common,source,exe,server,client,distributor,sink,grpcpp,request_source,test_common,test

which did not contain adaptive_load or benchmarks.

My understanding of this list is that we probably have more things here than we actually need. Anything omitted I believe gets bundled into one section and alphabetized within that section. This list is ordered, so will ensure that (for example) external/source/envoy appears before anything else in external. I assume that for some envoy-related thing that header order matters, but for most files/subdirectories it shouldn't, and it would probably be fine to have them bundled in one section.

I don't remember what it is, but we've typically added directories to this list upon some mysterious error that we have encountered on occasion, but I forget what it is, or if it is still relevant after this yaml migration.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you, feel free to merge in assuming the files in the adaptive_load directories do get formatted even though they aren't listed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We talked about this offline, but after running an experiment, I realized that we are in fact missing some features in those directories. I am going to follow up separately adding directories and fixing issues

@mum4k mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Sep 22, 2022
Signed-off-by: Nathan Perry <[email protected]>
@dubious90 dubious90 added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Sep 22, 2022
- envoy # unique
- nighthawk # unique
- external/source/envoy # unique
- external # unique
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you, feel free to merge in assuming the files in the adaptive_load directories do get formatted even though they aren't listed.

@mum4k mum4k assigned dubious90 and unassigned mum4k Sep 23, 2022
@mum4k mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Sep 23, 2022
@dubious90 dubious90 merged commit 73a7bf6 into envoyproxy:main Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-changes A PR waiting for comments to be resolved and changes to be applied.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix_format script always silently passes without formatting anything
3 participants