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

Pull Request? #321

Closed
MichaelLeeHobbs opened this issue Apr 24, 2023 · 4 comments
Closed

Pull Request? #321

MichaelLeeHobbs opened this issue Apr 24, 2023 · 4 comments

Comments

@MichaelLeeHobbs
Copy link

Are you interested in a pull request that is effectively a complete rewrite of the entire project in TypeScript? https://github.com/MichaelLeeHobbs/cron-parser

All tests pass and I've added around 150 more tests to get the code coverage up to 95.98%. I've got some more work to do but getting close to the end of phase 1. For phase 2 I was going to update the tests to Jest. If you are not interested in a pull request I'll rename the fork so as to not cause confusion.

@harrisiirak
Copy link
Owner

Hi @MichaelLeeHobbs!

Looks promising. I started with TS rewrite some time ago but haven't gotten that far. I'm more than happy to receive this as a PR. Although, reviewing this will be tedious work (but it's definitely doable) - actual TS conversion itself + some overall structural improvements.

Best regards

@MichaelLeeHobbs
Copy link
Author

MichaelLeeHobbs commented Apr 24, 2023

Sounds, good. I should be done by the end of next weekend then do the pull request. If I have time I'm going to review all the open bug tickets and see if I can close some out with this pr.

Edit: Not sure if you are aware but you can install the updated version for testing via: yarn add https://github.com/MichaelLeeHobbs/cron-parser it's fully functional as of now and I should not break it at this point.

@harrisiirak
Copy link
Owner

If I have time I'm going to review all the open bug tickets and see if I can close some out with this pr.

Solving some low-hanging bugs/issues would be a nice bonus if they don't create too much extra work. I do like what is going in your fork. However, I still prefer reviewing something rather concentrated and compact - if possible, I even consider somehow splitting this up into smaller pieces, so it makes sense while reviewing.

@MichaelLeeHobbs
Copy link
Author

MichaelLeeHobbs commented Apr 28, 2023

@harrisiirak I've added a bunch of tests for #156 see the end of https://github.com/MichaelLeeHobbs/cron-parser/blob/master/tests/CronExpression.test.ts I don't know if you had at some point fixed these or I accidentally fixed them in the refactor. They all seem to work correctly for me. The only issue I found was CronExpression.stringify does not return the original value and that would be very difficult to impossible. So I sort of cheated and added CronExpression.toString which return this.#expression || this.stringify(true); As long as this was a parsed expression then this.#expression will be the original expression.

I've also added a fix for #153 and #299.

Obviously, I still need to document and comment the code which I will do after I am done looking at issues.

Edit: Fixed #190, the refactor supports CJS and ESM.
Edit: Added a fix for #244 in the form of strict mode which defaults to false. See test should throw an error for expression missing fields when in strict mode (#244)

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