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

rename String.isEmpty to isNullOrEmpty #723

Open
subzero911 opened this issue Feb 6, 2023 · 3 comments
Open

rename String.isEmpty to isNullOrEmpty #723

subzero911 opened this issue Feb 6, 2023 · 3 comments

Comments

@subzero911
Copy link

subzero911 commented Feb 6, 2023

https://pub.dev/documentation/quiver/latest/quiver.strings/isEmpty.html

Because its actual implementation is s == null || s.isEmpty;
Also, it conflicts with Flutter standard library isEmpty function https://api.flutter.dev/flutter/dart-core/String/isEmpty.html which actually does what it says.

The same is applicable to isNotEmpty

@jtmcdole
Copy link
Member

jtmcdole commented Feb 6, 2023

Renaming global api, especially old api, is a breaking change. This was pre-null safe code (and pre Dart extensions). I'd suggest just removing it.

@subzero911
Copy link
Author

subzero911 commented Feb 6, 2023

Let's mark it @deprecated
And introduce a new isNullOrEmpty method.
The safiest way.

@cbracken
Copy link
Member

cbracken commented Feb 6, 2023

Oh wow yeah... this is pretty ancient stuff. Like @jtmcdole says, I'd be tempted to just deprecate it and not replace it. Prior to non-null by default, it was designed to solve usecases such as:

Iterable<String> maybeNonEmptyStrings = ...;
Iterable<String> nonEmptyStrings = maybeEmptyStrings.where(isNotEmpty);

With non-null by default (NNBD) the default behaviour now, and with the generic type of the returned Iterable of where matching the parameter type, this function isn't particularly useful any more in that scenario. This won't compile:

Iterable<String?> maybeNonEmptyStrings = ...;
Iterable<String> nonEmptyStrings = maybeEmptyStrings.where(isNotEmpty);

and this is undesirable:

Iterable<String?> maybeNonEmptyStrings = ...;
Iterable<String?> nonEmptyStrings = maybeEmptyStrings.where(isNotEmpty);

Arguably, with NNBD enabled, where you've got some code where nulls were explicitly permitted, it's better to be explicit about the filter criteria from a readability standpoint, so something like:

Iterable<String> nonEmptyStrings = maybeEmptyStrings
  .whereType<String>()
  .where((a) => a.isNotEmpty);

I'd suggest we just deprecate it and remove it in some future version.

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

No branches or pull requests

3 participants