Skip to content
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

documentation: Adds migration advice, removes no-op deprecations #861

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dkbast
Copy link

@dkbast dkbast commented Feb 13, 2023

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

…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;
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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,
Copy link
Owner

Choose a reason for hiding this comment

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

Ditto here. I'd like to keep these deprecated properties around for a while to ease migration.

Copy link
Author

Choose a reason for hiding this comment

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

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.

lib/svg.dart Outdated Show resolved Hide resolved
Comment on lines +168 to +169
/// The [ColorFilter] to apply to the picture. An easy way to add color.
/// e.g. `ColorFilter.mode(Colors.red, BlendMode.srcIn)`
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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 :)

@dkbast
Copy link
Author

dkbast commented Feb 13, 2023

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.
It was hard to tell if I can still use a deprecated field or if it just doesn't do anything anymore - If they would still have any effect I would 100% agree, but '@deprecated' implies that this is still working but will not in the future.

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

@felipecastrosales
Copy link

Very good, @dkbast. Congratulations on being proactive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants