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

Conversation

@stuartmorgan-g
Copy link
Contributor

Adds a new command that adds dependency_overrides to any packages in the repository that depend on a list of target packages, including an option to target packages that will publish a non-breaking change in a given diff.

Adds a new CI step that uses the above in conjunction with a new --run-on-dirty-packages to adjust the dependencies of anything in the repository that uses a to-be-published package and then re-run analysis on just those packages. This will allow us to catch in presubmit any changes that are not breaking from a semver standpoint, but will break us due to our strict analysis in CI.

Fixes flutter/flutter#89862

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. (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, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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.

I think "pathify" is is a bit difficult to grok. I know it's a pain to change in the PR but I think "convert_depenencies_to_relative_paths" would be more clear.

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.

The code LGTM. I read the change to the yml file first and didn't really understand what was happening until I read through the PR. I have the feeling that we are increasing the complexity more so we should do our best to make sure the documentation / naming is as clear as we can make it.

Copy link
Contributor Author

@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.

I think "pathify" is is a bit difficult to grok. I know it's a pain to change in the PR but I think "convert_depenencies_to_relative_paths" would be more clear.

I liked pathify, but you're right that clarity is more important than something that tries to be clever.

I went with make-deps-path-based; the important thing is that they are path-based, not that they are relative (I could just have easily have done absolute paths since people don't generally move their repos while testing a PR), and it's significantly shorter than convert-dependencies-to-path-based without sacrificing clarity.

@stuartmorgan-g
Copy link
Contributor Author

I also changed target-packages to target-dependencies (and similarly for the other flag) since I realized that using packages was ambiguous. I.e., "target packages" could just as easily mean the packages whose pubspecs are being rewritten, which isn't the case.

@stuartmorgan-g stuartmorgan-g changed the title [flutter_plugin_tools] Add a new 'pathify' command [flutter_plugin_tools] Add a new 'make-deps-path-based' command Dec 4, 2021
@stuartmorgan-g stuartmorgan-g merged commit 40572a7 into flutter:master Dec 4, 2021
@stuartmorgan-g stuartmorgan-g deleted the pathify branch December 4, 2021 17:43
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 4, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 4, 2021
amantoux pushed a commit to amantoux/plugins that referenced this pull request Dec 11, 2021
…ter#4575)

Adds a new command that adds `dependency_overrides` to any packages in the repository that depend on a list of target packages, including an option to target packages that will publish a non-breaking change in a given diff.

Adds a new CI step that uses the above in conjunction with a new `--run-on-dirty-packages` to adjust the dependencies of anything in the repository that uses a to-be-published package and then re-run analysis on just those packages. This will allow us to catch in presubmit any changes that are not breaking from a semver standpoint, but will break us due to our strict analysis in CI.

Fixes flutter/flutter#89862
KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021
…ter#4575)

Adds a new command that adds `dependency_overrides` to any packages in the repository that depend on a list of target packages, including an option to target packages that will publish a non-breaking change in a given diff.

Adds a new CI step that uses the above in conjunction with a new `--run-on-dirty-packages` to adjust the dependencies of anything in the repository that uses a to-be-published package and then re-run analysis on just those packages. This will allow us to catch in presubmit any changes that are not breaking from a semver standpoint, but will break us due to our strict analysis in CI.

Fixes flutter/flutter#89862
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.

Use path-based dependencies for analysis of federated plugins

2 participants