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

Why not splitting the parser from the expression? #112

Open
yanickrochon opened this issue Nov 2, 2017 · 3 comments
Open

Why not splitting the parser from the expression? #112

yanickrochon opened this issue Nov 2, 2017 · 3 comments

Comments

@yanickrochon
Copy link

I understand that some may need this, but given the triviality of the implementation, and how useless it is in the browser, why does this method actually exist in this package? Of course, who cares if it adds a few bytes when exported for the browser, but wouldn't a peer dependency be best suited for this method, as it is not a general use case?

Just wondering.

@yanickrochon yanickrochon changed the title Why CronParser.parseFile? Why not spiting the parser from the expression? Nov 2, 2017
@yanickrochon
Copy link
Author

Actually, why not creating two modules from this one? I actually only need the expression parser, and I got a little confused at the purpose of this module when looking at the README. If I am not mistaken, this module is about parsing crontab files / strings, while including the actual cron expression parser.

If both crontab parser and expression parser would be separate modules, my first comment would simply be invalidated.

If they were distinct modules, it would also be easier to maintain, IMO.

@harrisiirak harrisiirak changed the title Why not spiting the parser from the expression? Why not splitting the parser from the expression? Nov 2, 2017
@harrisiirak harrisiirak added this to the 3.0 milestone Nov 2, 2017
@harrisiirak
Copy link
Owner

harrisiirak commented Nov 2, 2017

Hi @yanickrochon! Thanks for your feedback.

I see your point keeping this module efficient and functional as possible. Although it's not adding much logic/weight to the module, it still needs attention when something changes or will be added. Definitely this is not happening in 2.x because it may break backward compatibility for some of the users. This something that can be done in 3.0.

My mind is open for this idea.

@yanickrochon
Copy link
Author

(Thank you for correcting the auto-correct 😛 )

Yes, I this is why I was linking to the other issue, and implementing the API for ES6 syntax, targeting Node 7 and upward. This would be for a next major release, as it would create significant enough API change to break some user code.

I would propose splitting this way :

  • cron-expression (it doesn't exist on npm, yet)
    contains the date and expression modules, parses a single cron-like string, retuns an interval object

  • cron-parser
    exposes two methods : parse and parseFile, where the latter invokes the former with a blob of text, and the returned value is an array of jobs :

    [
      { interval: [Object], command: [String], comments: [String] }
      ...
    ]
    

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants