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

Add boolean parse #51026

Closed
wants to merge 50 commits into from
Closed

Add boolean parse #51026

wants to merge 50 commits into from

Conversation

srburton
Copy link
Contributor

Hello, I see that many flutter users complain that they don't have a native parse for boolean, so I decided to create this feature.

@google-cla
Copy link

google-cla bot commented Jan 17, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@srburton
Copy link
Contributor Author

Syntax Java

import java.lang.*;

public class Program {

   public static void main(String[] args) {
      System.out.println(Boolean.parseBoolean("true")); //true
      System.out.println(Boolean.parseBoolean("false")); // false
      System.out.println(Boolean.parseBoolean("TRUE")); //true
      System.out.println(Boolean.parseBoolean("FALSE")); // false
      System.out.println(Boolean.parseBoolean("0")); //false
      System.out.println(Boolean.parseBoolean("1")); // false
      System.out.println(Boolean.parseBoolean("y")); // false
      System.out.println(Boolean.parseBoolean("n")); // false
   }
}

Syntax CSharp

using System;
					
public class Program
{
	public static void Main()
	{
		Console.WriteLine(bool.Parse("true")); // True
		Console.WriteLine(bool.Parse("false")); // False
		Console.WriteLine(bool.Parse("TRUE")); // True
		Console.WriteLine(bool.Parse("FALSE")); // False		
		Console.WriteLine(bool.Parse("0")); // FormatException
		Console.WriteLine(bool.Parse("1")); // FormatException
		Console.WriteLine(bool.Parse("y")); // FormatException
		Console.WriteLine(bool.Parse("n")); // FormatException
	}
}

Syntax Dart proposal

class Program
{
	static void Main()
	{
		print(bool.parse("true")); // True
		print(bool.parse("false")); // False
		print(bool.parse("TRUE")); // True
		print(bool.parse("FALSE")); // False		
		print(bool.parse("0")); // FormatException
		print(bool.parse("1")); // FormatException
		print(bool.parse("y")); // FormatException
		print(bool.parse("n")); // FormatException
	}
}
``

@mraleph
Copy link
Member

mraleph commented Jan 17, 2023

/cc @lrhn

@mraleph mraleph requested a review from lrhn January 17, 2023 11:42
sdk/lib/_internal/js_runtime/lib/js_helper.dart Outdated Show resolved Hide resolved
sdk/lib/_internal/js_runtime/lib/js_helper.dart Outdated Show resolved Hide resolved
sdk/lib/_internal/js_runtime/lib/js_helper.dart Outdated Show resolved Hide resolved
sdk/lib/core/bool.dart Outdated Show resolved Hide resolved
@lrhn
Copy link
Member

lrhn commented Jan 17, 2023

It's a valid addition. I'm not sure it's really valuable enough, but it's also very low-cost (static method, easily tree-shaken if not used).

I'd probably want the case-insensitivity to be optional, so

  external static bool parse(String source, {bool caseInsensitive = false});
  external static bool? tryParse(String source, {bool caseInsensitive = false});

Documentation should be clear that the only accepted inputs are the ASCII letter sequences "true" and "false", optionally allowing upper case letters as well as lower case, in any combination.

The important case is to recognize precisely the output of bool.toString(). Case insensitivity isn't important to me at all, and I'd be happy to not have it. If your input isn't from bool.toString(), it's custom input that could be anything.

On the other hand, without case insensitivity, it's so trivial a function that it's not particularly valuable.

The FormatException being thrown is not going to have a position, since being either true or false isn't really something that fails at a specific position (if input is "talse", where is the error?). So, not as helpful as you'd expect, unless we want to recognize case-differences and report them in case-sensitive mode.

@srburton
Copy link
Contributor Author

srburton commented Jan 17, 2023

@lrhn Thank you, I will make the suggested changes and add your caseInsensitive suggestion.

Just confirms me about the caseInsensitive is that what you imagine?

class Program
{
	static void Main()
	{	
              if(bool.parse("true")){
                //.. ok
              } 

              //FormatException
              if(bool.parse("FalSe", caseInsensitive: true)){  
              
              } 
	}
}

@lrhn
Copy link
Member

lrhn commented Jan 17, 2023

I'd expect no format exception when caseInsensitive is true.

It's a little iffy, because "case-insesitive" is a negative-like word, so it probably should be
bool caseSensitive = true as the default instead.
But a default of true that you change to false is also less than optimal.

I guess in this case, it's a trade-off. The more I look at it, I think I prefer

  external static bool? tryParse(String source, {bool caseSensitive = true});

Then bool.tryParse("true") is true, bool.tryParse("True") is null, and bool.tryParse("True", caseSensitive: false) is true as well.

@srburton
Copy link
Contributor Author

srburton commented Jan 18, 2023

@lrhn Could you check the new implementation?

Commit: 9974259

Syntax Dart proposal

void main() {
  
  print(bool.parse("true")); // true
  print(bool.parse("false")); //false
  print(bool.parse("TRUE")); // FormatException
  print(bool.parse("FALSE")); //FormatException
  print(bool.parse("True", caseSensitive: false)); // true
  print(bool.parse("False", caseSensitive: false)); // false
  
}

@rakudrama
Copy link
Member

Please add tests.

There are tests for other primitive classes, e.g. tests/corelib/double_parse_test.dart and tests/corelib/double_try_parse_test.dart.

This suggests tests/corelib/bool_parse_test.dart and tests/corelib/bool_try_parse_test.dart but I think it would be reasonable to test both methods in one file.

int.parse and double.parse are tolerant of surrounding whitespace. Should bool.parse be tolerant too?

I do not think that this function needs to have implementations specialized to the different runtimes.
Without a clear benefit (like using JavaScript number parsing on the web) this just adds maintenance burden.

Something like the following should be sufficient:

// sdk/lib/core/bool.dart

class bool {
  /// doc here
  bool parse(String source, {bool caseSensitive = true}) {
    return tryParse(source, caseSensitive: caseSensitive) ??
        (throw FormatError(...));
  }
  /// doc here
  bool? tryParse(String source, {bool caseSensitive = true}) {
    if (source == 'true') return true;
    if (source == 'false') return false;
    if (caseSensitive) return null;
    return tryParse(source.toLowerCase());
  }
}

I am not totally convinced that we should have case-insensitive parsing, but we do have case-insensitive parsing of numbers i.e. accepting numbers that would not be produced by some variant of toString().

@srburton
Copy link
Contributor Author

srburton commented Jan 18, 2023

@rakudrama @lrhn, I particularly like the C# implementation a lot, it works very well.

I believe that in dart we can do something like the example below:

void main() {

      print(bool.parse("true")); // true
      print(bool.parse("false")); //false
      print(bool.parse("TRUE")); // true
      print(bool.parse("FALSE")); // false
      print(bool.parse("  True  ")); // true
      print(bool.parse("  False  ")); // false
      print(bool.parse("")); // FormatException
      print(bool.parse("t r u e")); //FormatException
      print(bool.parse("truee")); //FormatException
      print(bool.parse("y")); // FormatException
      print(bool.parse("0")); // FormatException      
      print(bool.parse("1")); // FormatException
}

@lrhn
Copy link
Member

lrhn commented Jan 23, 2023

About the example:

void main() {
      print(bool.parse("true")); // true
      print(bool.parse("false")); //false
      print(bool.parse("TRUE")); // true
      print(bool.parse("FALSE")); // false
      print(bool.parse("  True  ")); // true
      print(bool.parse("  False  ")); // false
      print(bool.parse("")); // FormatException
      print(bool.parse("t r u e")); //FormatException
      print(bool.parse("truee")); //FormatException
      print(bool.parse("y")); // FormatException
      print(bool.parse("0")); // FormatException      
      print(bool.parse("1")); // FormatException
}

My experience from int.parse tells me that allowing leading and trailing whitespace is unnecessary and complicates the fast paths.
We have String.trim to remove the whitespace if you think you might have some, but if the parse method checks for whitespace, that's an extra overhead you can't avoid.

The one place where I'd have done something else for int.parse was that I'd allow it to parse inside an existing string, by passing in start and end, without having to create a substring. The latter is unnecessarily inefficient if you already know where the integer starts and ends in the string, which you need to know to create the substring.
That's not as important for bool.parse, because if you already know that you have a boolean substring, then just checking whether the length is 4 or 5 should tell you which one. For bool.parse, it's more likely that you will be using it on inputs that are already tokenized, and passing in the start/end in another string is not what you need.
(For performance, that would be bool parseInPlace(String source, int start, int end) => end == start + 4 ? source.startsWith("true", start) : source.startsWith("false", start);, which is a different thing.)

I'm a little split on whether case sensitivity should default to true or false. I'd like to say false to be as restrictive as possible by default, because being permissive can hide errors. But in this case, it's not like we will accept radically different inputs, and accepting True over true is only going to be a problem if you use the bool parser to validate something, like JSON, and why would you ever do that instead of using a proper tool for it like jsonDecode.
So true is probably OK.

@srburton
Copy link
Contributor Author

@rakudrama , @lrhn 779da89 add the tests, sorry for the delay.

sdk/lib/_internal/js_dev_runtime/patch/core_patch.dart Outdated Show resolved Hide resolved
sdk/lib/_internal/js_dev_runtime/patch/core_patch.dart Outdated Show resolved Hide resolved
sdk/lib/_internal/js_runtime/lib/js_helper.dart Outdated Show resolved Hide resolved
sdk/lib/_internal/js_runtime/lib/js_helper.dart Outdated Show resolved Hide resolved
sdk/lib/_internal/js_runtime/lib/js_helper.dart Outdated Show resolved Hide resolved
sdk/lib/core/bool.dart Outdated Show resolved Hide resolved
sdk/lib/core/bool.dart Outdated Show resolved Hide resolved
sdk/lib/core/bool.dart Outdated Show resolved Hide resolved
tests/corelib/bool_parse_test.dart Outdated Show resolved Hide resolved
tests/corelib/bool_parse_test.dart Outdated Show resolved Hide resolved
@lrhn
Copy link
Member

lrhn commented Jan 24, 2023

I also had a review I had forgotten to publish, so sorry about that delay too.

@mit-mit
Copy link
Member

mit-mit commented Feb 1, 2023

@srburton please see additional feedback in https://dart-review.googlesource.com/c/sdk/+/279746

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/279746 has been updated with the latest commits from this pull request.

1 similar comment
@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/279746 has been updated with the latest commits from this pull request.

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/279746 has been updated with the latest commits from this pull request.

1 similar comment
@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/279746 has been updated with the latest commits from this pull request.

@copybara-service copybara-service bot closed this in c974c70 Mar 9, 2023
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.

None yet

6 participants