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

Adding Support for custom handlers #27

Merged
merged 19 commits into from
Oct 22, 2024
Merged

Conversation

sean-parent
Copy link
Member

@sean-parent sean-parent commented Oct 14, 2024

  • Support for custom handlers (see ReadMe.md and new unit tests).
  • Some project cleanup: Removed unused features from the template.
  • Added support files for VSCode.
  • Some non-C++ files also got auto-linted.
  • Cleaned up a warning in the CMake (still two warnings left).

@sean-parent sean-parent requested a review from dabrahams October 14, 2024 22:05
@dabrahams dabrahams force-pushed the sean-parent/vscode-cleanup branch from f8ae395 to 1ad5373 Compare October 14, 2024 22:50
Copy link
Contributor

@dabrahams dabrahams left a comment

Choose a reason for hiding this comment

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

Looks like it should be about 4 PRs? See comments, some of which request changes.

- "-readability-avoid-const-params-in-decls"
- "-readability-else-after-return"
- "-readability-function-cognitive-complexity"
- "-readability-identifier-length"
Copy link
Contributor

Choose a reason for hiding this comment

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

Reasons for changes in this file are unclear. Remember that anything we're turning off to make our headers pass linting needs to be off in clients, so maybe we should be suppressing things with in-code comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed - I'll make a separate PR to clean these out or at least provide a rationale for anything left on.

Copy link
Member Author

Choose a reason for hiding this comment

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

See Issue #28.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
Dependencies.cmake Show resolved Hide resolved
test/custom_verbose_configuration_tests.cpp Outdated Show resolved Hide resolved
test/custom_verbose_configuration_tests.cpp Outdated Show resolved Hide resolved

}// namespace

#define STRINGIZE(x) STRINGIZE2(x)// NOLINT(cppcoreguidelines-macro-usage)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doc comments on these macros too please. If this is a better idiom for capturing the line number we should use it in the other tests too, right? Is it better?

Copy link
Contributor

Choose a reason for hiding this comment

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

is it better?

Copy link
Member Author

@sean-parent sean-parent Oct 15, 2024

Choose a reason for hiding this comment

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

It's the same idiom you use in verbose_configuration_tests.cpp - is it better than what?

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't recall having LINE_STRING, but since it's the same, we should DRY it, right?

test/custom_verbose_configuration_tests.cpp Outdated Show resolved Hide resolved
test/custom_verbose_configuration_tests.cpp Show resolved Hide resolved
@sean-parent
Copy link
Member Author

Looks like it should be about 4 PRs? See comments, some of which request changes.

I don't disagree - if you have some good tips about how to fix things when you see them in separate PRs with minimal friction, I'd love some suggestions. In my experience, several dependent PRs become very problematic.

@sean-parent sean-parent merged commit 9c2b5e6 into main Oct 22, 2024
@sean-parent sean-parent deleted the sean-parent/vscode-cleanup branch October 25, 2024 04:35
Comment on lines -15 to +12
clangFormatVersion: 12
inplace: True
- uses: EndBug/add-and-commit@v9
with:
author_name: Clang Robot
author_email: [email protected]
message: ':art: Committing clang-format changes'
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
- uses: actions/checkout@v4
with:
repository: ${{ github.event.pull_request.head.repo.full_name }}
ref: ${{ github.event.pull_request.head.ref }}
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these changes about?

@@ -1,3 +1,7 @@
{
"cmake.ctest.testExplorerIntegrationEnabled": true
"cmake.ctest.testExplorerIntegrationEnabled": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

This kind of change should get a comment saying it's a workaround and a reference to an issue, e.g.

// WORKAROUND: https://some/github/link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants