Skip to content

Conversation

@flutter-dashboard
Copy link

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.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Feb 24, 2021
@google-cla google-cla bot added the cla: yes label Feb 24, 2021
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a 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
Copy link
Contributor

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.

Copy link
Author

Choose a reason for hiding this comment

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

ahh!

Copy link
Member

@jmagman jmagman left a 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!

@Hixie
Copy link
Contributor

Hixie commented Mar 2, 2021

This change means that running the tests locally changes your local workspace.
I've applied those changes (I think) in #77093

fluttergithubbot pushed a commit that referenced this pull request Mar 3, 2021
zanderso added a commit that referenced this pull request Mar 19, 2021
zanderso added a commit that referenced this pull request Mar 19, 2021
zanderso added a commit that referenced this pull request Mar 20, 2021
zanderso added a commit that referenced this pull request Mar 20, 2021
zanderso added a commit that referenced this pull request Mar 23, 2021
zanderso added a commit that referenced this pull request Mar 23, 2021
zanderso added a commit that referenced this pull request Mar 23, 2021
zanderso added a commit that referenced this pull request Mar 23, 2021
zanderso added a commit that referenced this pull request Mar 23, 2021
* 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.
@stuartmorgan-g
Copy link
Contributor

This was apparently reverted at some point. Can we re-land it? what was the reason for the revert?

@Hixie
Copy link
Contributor

Hixie commented May 22, 2021

cc @zanderso who made the reverts (directly to the tree?)

@zanderso
Copy link
Member

A series of commits had to be reverted here #78623, to avoid the regressions described in
#78554 from appearing in a beta release.

@stuartmorgan-g
Copy link
Contributor

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.

@zanderso
Copy link
Member

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.

stuartmorgan-g added a commit to stuartmorgan-g/flutter that referenced this pull request May 24, 2021
Re-lands flutter#76662 updated for new
file location (and removing the Swift entry, since Swift doesn't use
clang-format).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants