[quick_actions] Add DartDoc and test coverage #2298
Conversation
| /// This is only exposed for the unit tests. Do not rely on it, it may break | ||
| /// without warning in the future. | ||
| @visibleForTesting | ||
| @deprecated |
There was a problem hiding this comment.
@collinjackson @diesersamat as far as I can tell this method isn't intended to be part of the public API, but I'm not sure if I'm misunderstanding the original intent here. From what I could tell reading through the codebase this looks like it's purely a helper method for intialize, and users of the plugin are never expected to call this on its own. I think it was originally added to help with testing. If my reading is wrong, what's the intended use case of this and calling getLaunchAction outside of initialize? It was added in #1800. if that helps.
Also adds a lint to prevent undocumented DartDocs from being added in the future. Some of the APIs were public when they really should have been `@visibleForTesting` or even private. Added some `@visibleForTesting` annotations as well.
collinjackson
left a comment
There was a problem hiding this comment.
lgtm with some minor nits
Co-Authored-By: Collin Jackson <jackson@google.com>
| /// This is only exposed for the unit tests. Do not rely on it, it may break | ||
| /// without warning in the future. | ||
| @visibleForTesting | ||
| @deprecated |
There was a problem hiding this comment.
Since we're making a breaking change anyway (removing channel) and this is a pre-1.0 plugin I think that we can probably skip the deprecated step and just remove this from the public API.
collinjackson
left a comment
There was a problem hiding this comment.
This is a minor nit but IMO when we make breaking changes we should be explicit in the CHANGELOG about exactly what changed, even if we think it's unlikely to affect anyone. Something like this:
- Added missing documentation.
- **Breaking change**. `channel` and `withMethodChannel` are now `@visibleForTesting`. These methods are for plugin unit tests only and may be removed in the future.
- **Breaking change**. Removed `runLaunchAction` from public API. This method was not meant to be used by consumers of the plugin.
|
@collinjackson good call, updated to use your wording. |
Also adds a lint to prevent undocumented DartDocs from being added in the future. Some of the APIs were public when they really should have been `@visibleForTesting` or even private. Added some `@visibleForTesting` annotations as well.
Description
Also adds a lint to prevent undocumented DartDocs from being added in
the future.
Some of the APIs were public when they really should have been
@visibleForTestingor even private. Added some@visibleForTestingannotations as well.
Related Issues
Fixes flutter/flutter#44399.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?