-
Notifications
You must be signed in to change notification settings - Fork 156
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
Comments
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 |
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. |
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. |
@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 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. |
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.
The text was updated successfully, but these errors were encountered: