-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[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
[file_selector] Add file group to save return value - platform interface #4254
Conversation
This is unchanged from the main PR except for the last commit that adds temp ignores. |
78ab0e4
to
381716f
Compare
There was a problem hiding this 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`. |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Platform interface portion of #4222
Part of flutter/flutter#107093
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).