Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[flutter_plugin_tools] Test and comment Dart analysis #4194

Merged

Conversation

stuartmorgan-g
Copy link
Contributor

Adds a unit test and comments intended to avoid accidental breakage of
the Dart repo's run of analysis against this repository.

Addresses #4183 (comment)

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

Adds a unit test and comments intended to avoid accidental breakage of
the Dart repo's run of analysis against this repository.

Addresses flutter#4183 (comment)
@stuartmorgan-g
Copy link
Contributor Author

@devoncarew How does this look?

@devoncarew
Copy link
Member

This looks good - I'm guessing that this will help in terms of reducing upstream breakages (or letting people know when we'll need to update things upstream).

I wonder if a good end state would be to allow a bare run of dart plugin_tools analyze to just work on the flutter/plugins (and flutter/packages) repos? We wouldn't have to know about specific analyze scripts in the repos, or, specific values of flags to pass in.

@stuartmorgan-g stuartmorgan-g requested a review from gaaclarke July 26, 2021 19:57
@stuartmorgan-g
Copy link
Contributor Author

I wonder if a good end state would be to allow a bare run of dart plugin_tools analyze to just work on the flutter/plugins (and flutter/packages) repos?

What is dart plugin_tools?

Comment on lines +11 to +15
# DO NOT move or delete this file without updating
# https://github.com/dart-lang/sdk/blob/master/tools/bots/flutter/analyze_flutter_plugins.sh
# which references this file from source, but out-of-repo.
# Contact stuartmorgan or devoncarew for assistance if necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make more sense to have this warning in the cirrus file next to the invocation there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good thought, that's more likely to be noticed in review. I left this longer explanation here, and added a shorter warning to .cirrus.yml that references it.

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

LGTM. This is definitely an improvement. I would have rather seen the cirrus script and the dart script share invocations so they can both be updated from this repo. Hopefully it wont be necessary after all this cleanup though.

@devoncarew
Copy link
Member

What is dart plugin_tools?

Ah, sorry, I'd meant flutter_plugin_tools, the tool in script/tool (I'd seen 'plugin_tool' from https://github.com/flutter/plugins/blob/master/script/common.sh#L11 but forget it was a bash function).

@stuartmorgan-g
Copy link
Contributor Author

The long term goal is not too have anything on custom analysis files, as previously discussed. I'm not sure how a totally bare invocation would work though; you added the SDK flag specifically for this use (IIRC because you preferred that to adjusting PATH, which would be the other way to run it).

@stuartmorgan-g stuartmorgan-g merged commit 31c598c into flutter:master Jul 26, 2021
@stuartmorgan-g stuartmorgan-g deleted the dart-repo-test-and-comments branch July 26, 2021 22:04
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 27, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 27, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 27, 2021
fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021
Adds a unit test and comments intended to avoid accidental breakage of
the Dart repo's run of analysis against this repository.

Addresses flutter#4183 (comment)
amantoux pushed a commit to amantoux/plugins that referenced this pull request Sep 27, 2021
Adds a unit test and comments intended to avoid accidental breakage of
the Dart repo's run of analysis against this repository.

Addresses flutter#4183 (comment)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants