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.

Sign up for GitHub

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

Jump to bottom

Simplify formatting scripts, add a clang-tidy script, and run clang-tidy #55785

Merged
merged 1 commit into from
Jan 29, 2022
Merged

Simplify formatting scripts, add a clang-tidy script, and run clang-tidy #55785

merged 1 commit into from
Jan 29, 2022

Conversation

nathanfranke
Copy link
Contributor

@nathanfranke nathanfranke commented Dec 10, 2021 •
edited
Loading

No description provided.

@nathanfranke nathanfranke requested review from a team as code owners December 10, 2021 12:57
@nathanfranke nathanfranke marked this pull request as draft December 10, 2021 12:59
@Chaosus Chaosus added this to the 4.0 milestone Dec 11, 2021
@nathanfranke nathanfranke mentioned this pull request Dec 17, 2021
Merged
@nathanfranke nathanfranke changed the title Update .clang-tidy and run clang-tidy Simplify formatting scripts, add a clang-tidy script, and run clang-tidy Dec 17, 2021
@nathanfranke nathanfranke marked this pull request as ready for review December 17, 2021 05:59
@fire
Copy link
Member

fire commented Jan 27, 2022

Are you able to update this?

@nathanfranke
Copy link
Contributor Author

Updated. Manual changes are now at https://github.com/nathanfranke/godot/tree/clang-tidy-manual for review.

@nathanfranke nathanfranke marked this pull request as draft January 27, 2022 18:11
@nathanfranke nathanfranke marked this pull request as ready for review January 27, 2022 20:00
akien-mga
akien-mga approved these changes Jan 28, 2022
View reviewed changes
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Left a couple comments, but everything else seems good 👍

@nathanfranke
Copy link
Contributor Author

@akien-mga In a future PR, should I add a switch to clang_tidy.sh to only check files changed since master/3.x, and use that switch in GitHub actions?

@akien-mga akien-mga merged commit 2f57a11 into godotengine:master Jan 29, 2022
@akien-mga
Copy link
Member

Thanks!

In a future PR, should I add a switch to clang_tidy.sh to only check files changed since master/3.x, and use that switch in GitHub actions?

That could be worth exploring, though in my experience clang-tidy is much more cumbersome to use and tends to create false positives/bugs, so I'm not sure I would want to make it mandatory to pass CI (since it would require that all contributors figure out how to run it locally on their changes).

@akien-mga akien-mga mentioned this pull request Jan 31, 2022
Merged
@akien-mga akien-mga mentioned this pull request Feb 1, 2022
Merged
akien-mga added a commit to akien-mga/godot that referenced this pull request Feb 1, 2022
@akien-mga
CI: Fix exclude patterns with git ls-files
56549a0 
Follow-up to godotengine#55785.

In `black_format.sh`, the `--exclude` switch was wrongly used. It's a misnomer
that only excludes _untracked_ files, arcane pathspec patterns should instead
be used to exclude _tracked_ files.

Using this newfound knowledge, we can also simplify the other scripts.
@nathanfranke nathanfranke deleted the clang-tidy branch February 2, 2022 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga approved these changes

Assignees
No one assigned
Labels
enhancement topic:buildsystem topic:core
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@nathanfranke @fire @akien-mga @Chaosus