[all] Cleanup unformatted files #7267
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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). 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. |
github-actions
bot
added
p: camera
p: local_auth
platform-ios
platform-macos
platform-windows
labels
| @@ -7,7 +7,7 @@ | |||
| NS_ASSUME_NONNULL_BEGIN | |||
|
|
|||
| /// Queue-specific context data to be associated with the capture session queue. | |||
| extern const char* FLTCaptureSessionQueueSpecific; | |||
| extern const char *FLTCaptureSessionQueueSpecific; | |||
There was a problem hiding this comment.
This probably depends on the exact version of clang-format, due to changes in the logic for detecting whether a file is C++ or Obj-C.
There was a problem hiding this comment.
I think I'm using the latest clang-format version. What version is CI using? Or has this file just not been touched in a really long time?
| @@ -39,7 +39,7 @@ CaptureControllerImpl::CaptureControllerImpl( | |||
| : capture_controller_listener_(listener), | |||
| media_settings_( | |||
| PlatformMediaSettings(PlatformResolutionPreset::kMax, true)), | |||
| CaptureController(){}; | |||
| CaptureController() {}; | |||
There was a problem hiding this comment.
All of the rest of these are minor differences in how clang-format handles this code, but the easy solution to these is to fix them all to not have the stray ; that shouldn't be there and is causing it to have non-trivial format analysis. Every one of these should have stable formatting across versions once the incorrect ; is gone.
There was a problem hiding this comment.
I removed the ; and the formatter still adds the space. Is that the expected behavior?
There was a problem hiding this comment.
Yes, the space is correct.
|
(FYI, the CI runs the same format command that you are running locally, and fails on diffs, so there are never files checked in that |
I figured that was the case. Is CI using an old clang-format version? Or does it only check files that changed in the PR. |
It's using a specific git revision specified in the CI config; I don't know the exact clang version it corresponds to.
Like almost all the repo commands, in CI it runs on any touched packages, or on all packages for repo-level changes (such as the daily Flutter roll). |
What is the reason for this? Should we update it? Should we update the documentation on how to setup the correct version of clang-format? I've accidentally checked in these formatting changes a few times and goofed up PRs. |
The reason for using specific builds in CI is to ensure repeatable behavior in CI.
If there's a newer CIPD package already available, I'd be happy to review a PR to update it. It was current as of about a year ago, and there hasn't been a need to update it.
It's not-trivial to set up an identical version locally, and this doesn't come up all that often. There's an issue tracking doing this IIRC, but it hasn't been a priority. I'm happy to review a PR for that as well. |
|
We should definitely do something about this. It's not a great developer experience (especially for new contributors) to have untouched files change by running the format command. |
|
@stuartmorgan See #7271 |
If clang is updated to a newer version that what all the contributors who don't currently have that problem are using, then they will start having that problem. There's no solution that will work for everyone unless the repo tooling manages a |
Yeah I'm thinking we just add a note to the readme for |
This repo uses clang 15 to format files. Newer versions of clang format code differently. This PR adds a note to the tool README about this and adds sample instructions for how to install clang 15 on macOS. We might want to add instructions for other platforms. Fixes flutter/flutter#152651 Additional context - #7267 - #7271
The format command always changes these even though I haven't touched them
flutter/flutter#152651
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].CHANGELOG.mdto add a description of the change, [following repository CHANGELOG style], or this PR is [exempt from CHANGELOG changes].///).