Feedback on parser for parsing durations

[venkatd]

I've written a parser and it works just fine. But wanted to get feedback to see if I'm following best practices.

A few things I feel a bit iffy about:

• I'm constantly casting things but it seems like Dart should know the type. For example with .toChoiceParser().cast<Duration>() it's already a list of Duration parsers
• I'm not sure if it's worth encapsulating the parsers inside a _buildX function. I thought I would make it lazily constructed, but if petitparser lazily compiles itself anyway, it might be overkill.
• I find myself constantly casting things in the map function
• I wonder if there are built-in helpers that I'm forgetting that could simplify some of the logic
• Maybe parseDuration and isValidDuration are unnecessary and I'm better off exposing Parser<Duration> directly for the rest of the app to use.

import 'package:petitparser/petitparser.dart';

import 'any_string.dart';

Parser<Duration> _buildDurationParser() {
  final hourUnit = anyStringIgnoringCase(['hours', 'hour', 'hrs', 'hr', 'h']);
  final minuteUnit =
      anyStringIgnoringCase(['minutes', 'minute', 'mins', 'min', 'm']);

  final numberWithDecimal =
      (char('0').or(digit().plus()) & char('.').seq(digit().plus())).flatten();

  final wholeNumber = (char('0').or(digit().plus())).flatten();

  final justDecimal = char('.').seq(digit().plus()).flatten();

  final number = [numberWithDecimal, wholeNumber, justDecimal]
      .toChoiceParser()
      .map((value) => double.parse(value as String));

  final hours =
      number.seq(whitespace().star()).seq(hourUnit).map<Duration>((value) {
    final hours = value[0] as double;
    final seconds = hours * Duration.secondsPerHour;
    return Duration(seconds: seconds.toInt());
  });

  final minutes =
      number.seq(whitespace().star()).seq(minuteUnit).map<Duration>((value) {
    final minutes = value[0] as double;
    final seconds = minutes * Duration.secondsPerMinute;
    return Duration(seconds: seconds.toInt());
  });

  final hoursAndMinutes =
      hours.seq(whitespace().star()).seq(minutes).map<Duration>((value) {
    final hours = value[0] as Duration;
    final minutes = value[2] as Duration;
    return hours + minutes;
  });

  return [hoursAndMinutes, hours, minutes].toChoiceParser().cast<Duration>();
}

final durationParser = _buildDurationParser();

Duration parseDuration(String durationString) {
  final result = durationParser.parse(durationString);
  return result.value;
}

bool isDurationValid(String durationString) {
  return durationParser.accept(durationString);
}

And my unit tests below:

import "package:test/test.dart";
import 'duration.dart';

void main() {
  group('parseDuration', () {
    test('basic hours', () {
      expect(parseDuration('0h'), Duration.zero);
      expect(parseDuration('1h'), Duration(hours: 1));
      expect(parseDuration('1 h'), Duration(hours: 1));
      expect(parseDuration('1 hour'), Duration(hours: 1));
      expect(parseDuration('1.5 hours'), Duration(hours: 1, minutes: 30));
      expect(parseDuration('.25 h'), Duration(hours: 0, minutes: 15));
    });

    test('basic minutes', () {
      expect(parseDuration('5 min'), Duration(minutes: 5));
      expect(parseDuration('30min'), Duration(minutes: 30));
      expect(parseDuration('90m'), Duration(hours: 1, minutes: 30));
    });

    test('hours and minutes', () {
      expect(parseDuration('5h30min'), Duration(hours: 5, minutes: 30));
      expect(
          parseDuration('5 hours 30 minutes'), Duration(hours: 5, minutes: 30));
    });
  });
}

Thanks!

[renggli]

1. There was a bug with the return type of the toChoiceParser, this has been fixed in the null-safety branch for a while: b5e85d6. Could probably be cherrypicked into older versions, but since this is potentially breaking existing code I am reluctant to release a new version.

2. Not sure what you mean by petitparser lazily compiles itself? I don't see anything going on lazily inside your _buildX function, though there is nothing particularly expensive either and it is only called once when the final variable is read first. Dart initializes final top-level variables lazily on the first read.

3. Casting things in the map function shouldn't be necessary. If your function returns R the returned parser is of type Parser<R>.

4. I don't see anything jumping out. Generally I would prefer using & and | operators for sequences and choices. There is a bit of a mix with seq and or, that are from a time where Dart didn't support custom operators yet. The advantage of using toSequenceParser and toChoiceParser is that they are a bit better at preserving the type information.

5. Depends on what you use it for? If other parts of the app want to reuse the parser, you could expose the parser; otherwise I think the API you built is simpler and doesn't force people to understand how PetitParser works.

[venkatd]

1. There was a bug with the return type of the toChoiceParser, this has been fixed in the null-safety branch for a while: b5e85d6. Could probably be cherrypicked into older versions, but since this is potentially breaking existing code I am reluctant to release a new version.

Makes sense. I upgraded to the null safety version and got some "unnecessary cast" links so confirming it's fixed on petitparser: ^4.0.0-nullsafety.1. Didn't realize that non null safe packages can depend on null safe packages 💯

2. Not sure what you mean by petitparser lazily compiles itself? I don't see anything going on lazily inside your _buildX function, though there is nothing particularly expensive either and it is only called once when the final variable is read first. Dart initializes final top-level variables lazily on the first read.

Didn't realize final vars are initialized on first read. So my _build function is unnecessary.

3. Casting things in the map function shouldn't be necessary. If your function returns R the returned parser is of type Parser<R>.

Got it thanks.

4. I don't see anything jumping out. Generally I would prefer using & and | operators for sequences and choices. There is a bit of a mix with seq and or, that are from a time where Dart didn't support custom operators yet. The advantage of using toSequenceParser and toChoiceParser is that they are a bit better at preserving the type information.

Makes sense. The preference is because the operators are easier to read right?

I like how the operators read too, but looks like I need explicit casts with them. For example:

final amPm = (am | pm).cast<MapEntry<_DTField, int>>();
// vs
final amPm = [am, pm].toChoiceParser();

Am I right to assume this is a current limitation of Dart's type inference?

5. Depends on what you use it for? If other parts of the app want to reuse the parser, you could expose the parser; otherwise I think the API you built is simpler and doesn't force people to understand how PetitParser works.

Good point.