alllex / parsus

Parser-combinators with Multiplatform Kotlin Coroutines
https://javadoc.io/doc/me.alllex.parsus/parsus-jvm/latest/index.html
MIT License
141 stars 4 forks source link

Add `parseEntireOrNull()` function #6

Closed aSemy closed 1 year ago

aSemy commented 1 year ago

I'd like to be able to parse and receive null if the parsing fails.

Remove parseEntireOrThrow()?

I'd even go further and completely remove the parseEntireOrThrow() function.

Document parseEntireOrThrow() with @Throws

Because I'm proposing removing parseEntireOrThrow() I thought I'd put this issue here rather than make a new issue.

I'm using Parsus in a .kts script, and unfortunately IntelliJ doesn't handle dependencies very well, so I can't see the source code documentation that says what exception parseEntireOrThrow() could throw.

image image

Adding @Throws would help with JVM consumers.

alllex commented 1 year ago

I think that's a good suggestion. This could work for cases where it's fine to tell the end-user that the complete input is not valid.

Overall, however, it's also important to provide some means of localized error reporting. If parsing fails, it may be better to say that in the particular place (position in the input) something else was expected. This information gets lost, if we just do parseEntireOrNull.

aSemy commented 1 year ago

Thanks for looking into it @alllex.

I've had a bit more of a play around and I agree, having the failure reasons would be important.

I can see that this functionality already exists:

SemVerParser.parseEntire(input).getOrElse { return null }

But because it requires chaining a couple of methods it's a little harder to find.

To make this more similar to the Kotlin stdlib (e.g. getOrElse(), I propose introducing a parseOrElse() {} function.

SemVerParser.parseOrElse(input) { err -> 
  error("could not parse $input, because $err")
}

// or return another value
val DEFAULT = SemVer("0.0.0-SNAPSHOT")
SemVerParser.parseOrElse(input) { DEFAULT }

And then parseOrNull() would be a shortcut for

SemVerParser.parseOrElse(input) { null }

which might be useful in some situations.

I also suggest removing Entire from the function names. There's no 'partial' parsing, all parse* functions parse the entire input, so I think the 'Entire' keyword is a little confusing and redundant. If partial parsing was introduced, then it should be done with an overload with a range.

alllex commented 1 year ago

I agree that OrElse and OrNull shorthands are idiomatic in Kotlin. I'll add them soon.

As for the Entire, I would keep it for now to keep the intent clear as it is done with Regex APIs in Kotlin.

alllex commented 1 year ago

After more consideration, I actually agree with you that Entire seems like an overkill. I will introduce functions without it in the name, and deprecate the ones that have it.

alllex commented 1 year ago

All functions you described are on main now and will be included in the next release

aSemy commented 1 year ago

Thanks for the updates @alllex!