-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Disable clang format in the plugin registrants #76662
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
Disable clang format in the plugin registrants #76662
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, interesting. I was thinking we would turn this off via filtering in the flutter/plugins autoformat tooling.
On one hand, it feels odd to make declarations about how the clients should (or in this case, shouldn't) format the generated code, but on the other hand I can't think of likely scenarios where someone would actually want to autoformat code that's going to be stomped on every build.
I guess I come down on the side of LGTM (modulo CMake), but let's get a second opinion.
| # Generated file, do not edit. | ||
| # | ||
| # clang-format off |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang format shouldn't be running on CMake files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh!
jmagman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than @stuartmorgan's cmake comment, if it works, LGTM!
|
This change means that running the tests locally changes your local workspace. |
* Revert "Enable dart_plugin_registry_test (#76645)" This reverts commit 109e0bb. * Revert "Apply changes caused by #76662 (#77093)" This reverts commit cdca648. * Revert "Disable clang format in the plugin registrants (#76662)" This reverts commit dadbd47. * Revert "Disable warnings for the dart plugin registrant (#76561)" This reverts commit 098ece5. * Revert "Remove dart_plugin_registry_test timeouts (#76838)" This reverts commit 1610a27. * Revert "Implement dartPluginClass support for plugins (#74469)" This reverts commit b7d4806. Kick.
|
This was apparently reverted at some point. Can we re-land it? what was the reason for the revert? |
|
cc @zanderso who made the reverts (directly to the tree?) |
|
This PR wasn't related to that at all, which is why I was confused as to why it was reverted. I guess it just got caught in the net by accident. |
|
Yeah, sorry about not broadcasting the reverts more widely. I just cc'd @blasten there. I believe this change did conflict with a revert that was related, so it unfortunately got swept in with the others. |
Re-lands flutter#76662 updated for new file location (and removing the Swift entry, since Swift doesn't use clang-format).
As per https://clang.llvm.org/docs/ClangFormatStyleOptions.html#disabling-formatting-on-a-piece-of-code