dart-lang / markdown

A Dart markdown library
https://pub.dev/packages/markdown
BSD 3-Clause "New" or "Revised" License
450 stars 201 forks source link

Add deprecated accessors for indicatorForUncheckedCheckBox and indicatorForCheckedCheckBox #459

Closed timmaffett closed 2 years ago

timmaffett commented 2 years ago

This PR adds back in the obsolete/removed indicatorForUncheckedCheckBox and indicatorForCheckedCheckBox symbols. They have been marked deprecated and changed to empty strings to reflect that the previous invisible non-breaking space containing symbols are not used internally to trigger checkboxes, so they will not be found in the output of markdown.

I would love any suggestions for better deprecated messages for these two, as well as any other suggestions.

Please see detailed discussion at https://github.com/dart-lang/markdown/pull/450 .

By now returning empty strings this should allow any test code using these symbols to predict the expected output of checkbox markup to continue to work unchanged with the new checkbox scheme that does not any invisible non-breaking space symbols.

This change alone allows the current version of flutter_markdown to compile correctly and for all tests to compile and pass.

(I have an additional pr for flutter_markdown that completely removes their use, but this is for any other users of these symbols. I would expect that any use of these would have been for testing similar to how flutter_markdown used them, and this change prevent the new checkbox scheme from being a braking change.)

cyanglaz commented 2 years ago

flutter/packages is using indicatorForUncheckedCheckBox and `indicatorForCheckedCheckBox, which causes ci to fail: https://cirrus-ci.com/task/5927849017212928?logs=analyze#L370

What is the suggested fix here? Is there a replacement of those markdowns that we can use?

srawlins commented 2 years ago

You could probably use '' as a replacement.

timmaffett commented 2 years ago

The ci is failing because of the --fatal-infos is causing a fail when the deprecation warnings occur.

https://github.com/flutter/packages/pull/2613 is the modification to flutter_markdown (within flutter/packages) which will remove these deprecation warnings. The PR is basically remove the use of these symbols within flutter_markdown.

The deprecated versions return empty strings - flutter_markdown will pass it's test with the empty string versions (which are causing the deprecation warning), but it is the warning that is causing ci to fail.

As @srawlins suggests empty strings ('') will make it work (tests pass), but the ci is pulling in the new version of markdown that returns the empty strings and but ALSO has the deprecation warnings; in your case it is the deprecation warnings that are breaking the ci.

(excerpt from ci output):

running command: "dart analyze --fatal-infos" in /tmp/cirrus-ci-build/packages/flutter_markdown
Analyzing flutter_markdown...
   info - test/list_test.dart:9:10 - 'indicatorForCheckedCheckBox' is deprecated and shouldn't be used. This string is no longer used internally.  It be will be removed in a future version.. Try replacing the use of the deprecated member with the replacement. - deprecated_member_use
   info - test/list_test.dart:9:39 - 'indicatorForUncheckedCheckBox' is deprecated and shouldn't be used. This string is no longer used internally.  It will be removed in a future version.. Try replacing the use of the deprecated member with the replacement. - deprecated_member_use
   info - test/list_test.dart:134:14 - 'indicatorForCheckedCheckBox' is deprecated and shouldn't be used. This string is no longer used internally.  It be will be removed in a future version.. Try replacing the use of the deprecated member with the replacement. - deprecated_member_use
   info - test/list_test.dart:136:14 - 'indicatorForUncheckedCheckBox' is deprecated and shouldn't be used. This string is no longer used internally.  It will be removed in a future version.. Try replacing the use of the deprecated member with the replacement. - deprecated_member_use
   info - test/list_test.dart:182:14 - 'indicatorForCheckedCheckBox' is deprecated and shouldn't be used. This string is no longer used internally.  It be will be removed in a future version.. Try replacing the use of the deprecated member with the replacement. - deprecated_member_use
   info - test/list_test.dart:184:14 - 'indicatorForUncheckedCheckBox' is deprecated and shouldn't be used. This string is no longer used internally.  It will be removed in a future version.. Try replacing the use of the deprecated member with the replacement. - deprecated_member_use
cyanglaz commented 2 years ago

Is there a replacement for "indicatorForUncheckedCheckBox" instead of an empty string? I'm not familiar with flutter_markdown but I imagine "indicatorForUncheckedCheckBox" previous shows a character that looks like a check box. I'd like to keep the behavior the same if possible.

timmaffett commented 2 years ago

The symbols indicatorForUncheckedCheckBox and indicatorForCheckedCheckBox were previously used internally while parsing the markup to indicate a place where a checkbox would be inserted in a subsequent step. These strings where 1 or 2 of the Unicode invisible non breaking space characters. The symbols were visible externally primarily for test purposed because they would be still present in the output strings. So test programs could use the strings to create the 'expected output' strings for comparison against what markdown was outputting.. There was really no other use for them - if these indicators were present in the source text they would have been ignored, they had to have been inserted by the markdown code itself. I think the only viable use for the symbols was for testing.

The reason they were changed to empty strings was to allow test programs, that were using the strings to calculate 'expected output', to still work. (Because the invisible indicators were no longer used internally test programs which were inserting the strings to predict output now needed to inserted nothing, and hence the empty strings).

The new checkbox markdown algorithm is entirely different and no longer requires these invisible place holders.

From the markdown user's point of view, that is creating markdown source using standard markdown syntax, nothing has changed. You still write the same markdown:

 - [ ] Checkbox not checked
 - [X] Checkbox checked

and it creates

cyanglaz commented 2 years ago

I see, looks like replacing these constants with '' is the way we want to go with to make CI happy.