Skip to content

[file_selector] Add file group to save return value - platform interface #4254

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

Conversation

stuartmorgan-g
Copy link
Contributor

Platform interface portion of #4222

Part of flutter/flutter#107093

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/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package 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.

@stuartmorgan-g
Copy link
Contributor Author

This is unchanged from the main PR except for the last commit that adds temp ignores.

@stuartmorgan-g stuartmorgan-g force-pushed the file-selector-save-extension-group-platform-interface branch from 78ab0e4 to 381716f Compare June 20, 2023 15:32
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

Minor comment about maybe making this a major version change.

@@ -1,3 +1,7 @@
## 2.6.0

* Adds `getSaveLocation` and deprecates `getSavePath`.
Copy link
Member

Choose a reason for hiding this comment

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

If this were a major version bump, wouldn't it make this PR much smaller, without really affecting much further ones down the line? (next you'll implement and use the new method, and remove the ignores, but wouldn't it be ~the same amount of work to bump the minimum version of the dependency of this package in the others?)

Copy link
Member

Choose a reason for hiding this comment

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

(Also, do the "ignores" need to be published to the changed packages?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Major version bumps to platform interfaces are unpleasant if there are any third-party implementations, and enabling those is one of the main goals of federation, so we try to minimize them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Also, do the "ignores" need to be published to the changed packages?)

Nope, ignores are dev-only changes. I need to teach the tooling to do line-level analysis for the version and changelog checks, and ignore non-doc comments.

Copy link
Member

Choose a reason for hiding this comment

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

Major version bumps to platform interfaces are unpleasant if there are any third-party implementations, and enabling those is one of the main goals of federation, so we try to minimize them.

As an alternative, can you deprecate getSavePath as a last change to the platform interface, once that every package has been updated to use the new version? Otherwise we're going to be yelling at users without them having an available solution, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As an alternative, can you deprecate getSavePath as a last change to the platform interface, once that every package has been updated to use the new version?

Our previous workflow was to in theory do it at the end, but in practice we just always forget and nothing gets deprecated :( And we don't even always remember to update all of our own code to use the new methods. So we're trying this flow instead to see how it goes.

Otherwise we're going to be yelling at users without them having an available solution, right?

Only users who are running analysis on our packages, which I would expect to be nobody.

Copy link
Member

Choose a reason for hiding this comment

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

Our previous workflow was to in theory do it at the end, but in practice we just always forget and nothing gets deprecated :( And we don't even always remember to update all of our own code to use the new methods. So we're trying this flow instead to see how it goes.

git push --force then! :P

@stuartmorgan-g stuartmorgan-g added override: no versioning needed Override the check requiring version bumps for most changes override: no changelog needed Override the check requiring CHANGELOG updates for most changes autosubmit Merge PR when tree becomes green via auto submit App labels Jun 21, 2023
@auto-submit auto-submit bot merged commit 9af50d4 into flutter:main Jun 21, 2023
@stuartmorgan-g stuartmorgan-g deleted the file-selector-save-extension-group-platform-interface branch June 22, 2023 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App override: no changelog needed Override the check requiring CHANGELOG updates for most changes override: no versioning needed Override the check requiring version bumps for most changes p: file_selector platform-linux platform-macos platform-windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants