documentation: Adds migration advice, removes no-op deprecations #861
Conversation
…ons and add deprecation messages with suggestions on how to change the code
| /// If this is null, the value in [SvgPicture.cacheColorFilter] is used. If it | ||
| /// is not null, it will override that value. | ||
| @Deprecated('This no longer does anything.') | ||
| bool? cacheColorFilterOverride; |
There was a problem hiding this comment.
Please don't remove these. There are usages of this API in some customers and removing them makes it much harder to migrate those customers to the new version.
There was a problem hiding this comment.
Especially with the doc comment still pointing towards this "doing something" this is quite confusing.
| @@ -87,8 +71,6 @@ class SvgPicture extends StatelessWidget { | |||
| this.semanticsLabel, | |||
| this.excludeFromSemantics = false, | |||
| this.theme = const SvgTheme(), | |||
| @deprecated Clip? clipBehavior, | |||
There was a problem hiding this comment.
Ditto here. I'd like to keep these deprecated properties around for a while to ease migration.
There was a problem hiding this comment.
Hmm ... I would actually argue that they actually hinder migration, since by keeping them as noop it's not safe to assume that we can "ignore" deprecation warnings as in this case the deprecation already happened.
| /// The [ColorFilter] to apply to the picture. An easy way to add color. | ||
| /// e.g. `ColorFilter.mode(Colors.red, BlendMode.srcIn)` |
There was a problem hiding this comment.
Here and elsewhere: docs should contain complete sentences thatstart with a capital letter and end with a period.
It would be great to expand this a bit to mention that it is also possible to use the ColorMapper property which avoids filtering at runtime and instead replaces colors at parse time.
There was a problem hiding this comment.
Hey - thats sounds great! I'm currently not more knowledgable to suggest anything more than what I needed to do my migration, but please feel free to improve upon this - the PR is open for Maintainer edits :)
|
Hi @dnfield - thanks for the quick review! I just did a migration on a bigger project and having many no-ops around made it a lot more difficult to migrate. From the docs: "The intent of the @deprecated annotation is to inform authors who are currently using the feature, that they will soon need to stop using that feature in their code, even if the feature is currently still working correctly." Please let me know if you still want me to reintroduce the dead code :) - no offence meant, the migration just did not go as smooth as you probably hoped to achieve |
|
Very good, @dkbast. Congratulations on being proactive. |
documentation: Add link to migration guide and remove no-op deprecations and add deprecation messages with suggestions on how to change the code.
Adds the tipps from #852 to the migration document and adds #828 to the deprecation notice so you don't have to look around for the fix.
Tagging @dnfield for review