Promote Atto parser as a module

[mbaechler]

This PR moves the current parser (using parser-combinators) into a new module and makes it possible to implement other parsers.
Then it moves atto parser as a module and checks that parser tests are giving the same outputs with both parsers.

This PR is a draft because there's a difficult choice to make.

At this point, users have to add a new dependency (parserc) to their projects to use Cron class as previously.

There are two solutions.

1/
We could accept that change and write a nice migration documentation, even write a scala steward rule. After all, it's just an additional dependency to add.

2/
The other solution I can see is to rename core into even-more-core and change core to be an aggregate of even-more-core and parserc.

This solution creates two new dependencies for our users but it would be transparent to them.

We would then just have to find a decent name for even-more-core.

@alonsodomin What do you think?

[alonsodomin]

Looks good so far, only minor nitpicking from my side

[alonsodomin]

nit: don't think we need the @FunctionalInterface annotation here, as it's meant for Java interaction only and don't expect anybody trying to implement their own parsers in Java, considering the return type is Either.

Also, for more FP-idiomatic style, the trait could be defined as:

type Parser = String => Either[Error, CronExpr]

[mbaechler]

Well, I had a warning (promoted as compilation error on CI) asking me to put this annotation.
I can change it for a type definition if it matters.

[mbaechler]

I did the change here b1a5571

[mbaechler]

Looks good so far, only minor nitpicking from my side

Do you have an opinion about the migration path for users?

[alonsodomin]

Do you have an opinion about the migration path for users?

I would simply make the new Atto parser the default but making it such that source code backwards compatibility is maintained. Meaning that a bumping the cron4s version downstream doesn't require any code changes.

However, at least for one or two versions, keep the legacy scala-parser-combinators implementation, which could be simply enabled by having an extra parameter in the Cron.parse methods, something like useLegacyParser: Boolean = false. This should be documented in the docs, there is migration folder where that should be documented.

This will in fact break binary compatibility so may require a major version bump. i.e.: 0.5.x

[mbaechler]

Do you have an opinion about the migration path for users?

I would simply make the new Atto parser the default but making it such that source code backwards compatibility is maintained. Meaning that a bumping the cron4s version downstream doesn't require any code changes.

That seams fair.

However, at least for one or two versions, keep the legacy scala-parser-combinators implementation, which could be simply enabled by having an extra parameter in the Cron.parse methods, something like useLegacyParser: Boolean = false. This should be documented in the docs, there is migration folder where that should be documented.

This one is more a problem. My main goal is to be able to remove the dependency to scala-parser-combinators lib. So keeping the implementation in the same module won't work for me.

However, I can make Atto the default and put the scala-parser-combinators version in another module and document how to use it.

I'm not sure which problem it solves though.

This will in fact break binary compatibility so may require a major version bump. i.e.: 0.5.x

Ok.

[alonsodomin]

I don't think that keeping the old parser solves any problem in particular more than potentially prevent it. While it's great that we have tests that we can use to validate both implementations there may be some cases in which the behavior of each the two parsers still differs somehow and I find it polite towards end users to be able to opt out of the new parser, at least in the next version. In following versions the old parser (and opt-out flag) would be removed altogether.

I don't mind if the old parser is moved to its own module or kept as is, probably the former is a cleaner implementation. If in a separate module it could also be argued that now the end user needs to choose which parser to use by importing the specific parser module (instead of using a flag to opt-out).

However I personally put priority on end user experience and still believe that having a default parser is a much better experience as a consumer of the library, also it allows to a smooth version upgrade: imagine an scenario in which Scala Steward creates an upgrade PR in someone else's repo, if the PR fails due to missing dependencies other users are less likely to look deeply into it and upgrade.

Regarding dragging around the dependency on Scala parser combinators, there are ways to make it in the use site, for example if we mark it with % Optional in our dependencies then it should need to be explicitly included downstream if users prefer that.

I mean, the usage of flag is not a hard requirement, just a quick idea thrown out. I'm open to there strategies that allow end users to opt-out of the new parser, just would like it to be documented and not being super-complicated to do.

[mbaechler]

Thank you for your detailed reply.

I thing that making Atto the default implementation and putting parser-combinators version in its own module fulfill your end user requirement : for most users, the upgrade will just work, and for people wanting the previous parser, they import the dependency and explicitly instantiate the parser.

I will have a look next week and propose a non-draft PR.

[mbaechler]

@alonsodomin I updated the PR. Now the core module depends on atto which itself depends on domain.
The code previously in core is now in domain.
core is effectively an empty module that pulls atto to make the transition smooth for users.

I have a (hopefully) last problem: Atto does not have a native version, so I can't build a native core module that uses atto.

I can have some version specific code in native directory to make Cron use the parser-combinator parser but I don't know how to specify that core depends on atto for js and jvm but on parserc on native. Do you have a clue how to implement that?

[alonsodomin]

Hello, specifying different dependencies for each target platform is super easy, you need to use .jsSettings(...), .jvmSettings(...) or .nativeSettings(...) and put the libraryDependencies setting for the specific platform in there.
https://github.com/portable-scala/sbt-crossproject

Regarding this refactoring, need to sit through it, as a start not a big fan of it, or even the naming domain chosen..., much less of having an almost empty module.

Let's look at the codebase in funcional point of view: The parser itself doesn't depend on all of the classes/traits that have been moved. In fact it should only require the types defined in the expr package. There are couple of offenders in this case: CronField and CronUnit as those are required by the expr package but both are defined in the root package. All the logic that calculates future dates operates on the CronExpr type, which gets constructed after parsing and therefore it doesn't directly depend on a parser implementation, nor the parser needs any of that to.

My preferred approach would be to make a clean cut between what is required to construct the CronExpr, and _what we can do once we have a CronExpr. That leave two modules that actually provide with functionality on their own, one has all the dependencies needed by the parsers (and nothing else) and core keeps all the lib logic that operates on CronExpr. Regarding naming, the new module can be named as cron4s-kernel, it's lame but it's a common practice in other projects.

There is an extra issue I see: the Cron singleton object. This was from the origin always intended as an easy entry point into the parser but the way it is right now it will create a bytecode conflict for the cases in which someone wants to opt-out of the Atto parser. This is because both the core module and the parsec module define the same cron4s.Cron package. This needs extra thought as regardless if the modules being used by end users, there must be only one definition of this type. A potential solution is that, instead of instantiating the singleton with a parser, the individual methods in the Cron type take an implicit Parser type as extra parameter. This parameter can have a default that points to the Atto parser but should be easily overridden when doing import cron4s.parser.Parsec._ providing such a package or object provides with the appropriate implicit instance.

[mbaechler]

Thanks for your feedback.

Hello, specifying different dependencies for each target platform is super easy, you need to use .jsSettings(...), .jvmSettings(...) or .nativeSettings(...) and put the libraryDependencies setting for the specific platform in there. https://github.com/portable-scala/sbt-crossproject

Regarding this refactoring, need to sit through it, as a start not a big fan of it, or even the naming domain chosen..., much less of having an almost empty module.

Let's look at the codebase in funcional point of view: The parser itself doesn't depend on all of the classes/traits that have been moved. In fact it should only require the types defined in the expr package. There are couple of offenders in this case: CronField and CronUnit as those are required by the expr package but both are defined in the root package. All the logic that calculates future dates operates on the CronExpr type, which gets constructed after parsing and therefore it doesn't directly depend on a parser implementation, nor the parser needs any of that to.

My preferred approach would be to make a clean cut between what is required to construct the CronExpr, and _what we can do once we have a CronExpr. That leave two modules that actually provide with functionality on their own, one has all the dependencies needed by the parsers (and nothing else) and core keeps all the lib logic that operates on CronExpr. Regarding naming, the new module can be named as cron4s-kernel, it's lame but it's a common practice in other projects.

That would work for me

There is an extra issue I see: the Cron singleton object. This was from the origin always intended as an easy entry point into the parser but the way it is right now it will create a bytecode conflict for the cases in which someone wants to opt-out of the Atto parser. This is because both the core module and the parsec module define the same cron4s.Cron package.

You probably misread, it's actually cron4s.parserc.Cron

This needs extra thought as regardless if the modules being used by end users, there must be only one definition of this type. A potential solution is that, instead of instantiating the singleton with a parser, the individual methods in the Cron type take an implicit Parser type as extra parameter. This parameter can have a default that points to the Atto parser but should be easily overridden when doing import cron4s.parser.Parsec._ providing such a package or object provides with the appropriate implicit instance.

Well, that's another way to do it. I opted for the explicit approach : the user uses cron4s.Cron for the default behavior and cron4s.parserc.Cron for the rare cases it's needed. The main reason for this strategy was a transparent migration.

If you think the implicit solution is better, let me know and I'll implement it.

[mbaechler]

Thanks for your feedback.

Hello, specifying different dependencies for each target platform is super easy, you need to use .jsSettings(...), .jvmSettings(...) or .nativeSettings(...) and put the libraryDependencies setting for the specific platform in there. https://github.com/portable-scala/sbt-crossproject
Regarding this refactoring, need to sit through it, as a start not a big fan of it, or even the naming domain chosen..., much less of having an almost empty module.
Let's look at the codebase in funcional point of view: The parser itself doesn't depend on all of the classes/traits that have been moved. In fact it should only require the types defined in the expr package. There are couple of offenders in this case: CronField and CronUnit as those are required by the expr package but both are defined in the root package. All the logic that calculates future dates operates on the CronExpr type, which gets constructed after parsing and therefore it doesn't directly depend on a parser implementation, nor the parser needs any of that to.
My preferred approach would be to make a clean cut between what is required to construct the CronExpr, and _what we can do once we have a CronExpr. That leave two modules that actually provide with functionality on their own, one has all the dependencies needed by the parsers (and nothing else) and core keeps all the lib logic that operates on CronExpr. Regarding naming, the new module can be named as cron4s-kernel, it's lame but it's a common practice in other projects.

That would work for me

As far as I understand, there's no ADT-like to express Cron expressions. The parsers are instantiating some Nodes which already contain logic about what we can do about them.

Implementing this plan would require to:

1. define an ADT representing a CronExpr (in kernel parser module)
2. once parsed, we transform this ADT into the current Node types (in core module)

It's should not be too hard but before I dive into it, do you think there's a better plan?
Is it what you meant in the first place @alonsodomin ?

[mbaechler]

With the last commit, we now have a parser module that provides just enough types to write a parser.
Then atto and parserc are implemented in term of cron4s.parser.
And finally, core module produces the usual cron4s.expr types from cron4s.parser ones.
@alonsodomin Let met know if this looks fine to you.

[mbaechler]

@alonsodomin Did you have the chance to review my last proposal for this PR?

[alonsodomin]

@mbaechler just took a quick look and it doesn't look bad at all, I like much more than the previous iteration. I have a few observations though, mostly nitpicking:

• It would be good to annotate the old parser as @deprecated
• We need to have documentation mentioning these changes and providing an example on how to use the old parser (warning users that it will be removed in following versions).
• Not fully obvious to me how users could override the default parser in the Cron entry point object, maybe should it have a def withParser(parser: Parser): Cron method to make it handy?
• the ParserAdapter looks like it should be package private API

Other than that, great job on this refactoring! 👍

[mbaechler]

All comments have been handled, you can check one more time @alonsodomin

[alonsodomin]

LGTM 👍