-
Notifications
You must be signed in to change notification settings - Fork 78
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
Refactored validateJob method complexity #56
Refactored validateJob method complexity #56
Conversation
No clue why tests are failing 🤔 they pass locally, but that just might be my modified configuration. |
Codecov Report
@@ Coverage Diff @@
## master #56 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 4 +3
Lines 297 368 +71
=========================================
+ Hits 297 368 +71
Continue to review full report at Codecov.
|
a814868
to
bbe1332
Compare
931d2c8
to
7bbb690
Compare
@niftylettuce I've finished the refactor I had in mind. Was wondering what's your opinion on use of JSDocs? I think it would help to somewhat document what each utility does and even assist with some type checking in the IDE. |
I've added as much coverage as possible but can't find the last piece to cover last 3% Currently nyc reports:
And the |
58680ca
to
4cec467
Compare
@niftylettuce this PR is ready to be reviewed. The test are failing for the same flaky reason as they do in master. The coverage is 100% on Node v10. Let me know what you think about the changes, happy to break them down a little if that's to much to review (tried to do incremental changes with each commit instead of one bulk of changes) |
@naz When I pulled into my local environment all of the tests worked, however this is what I got back on the coverage:
|
@shadowgate15 I think the problem with coverage is the same one as in current master - it works for Node v10 but then fails for Node v12. I have not introduced any new pathways, so the coverage should stays the same. |
Wow! I just re-ran the tests and got a completely different coverage... looks like I really need to sit down with the tests and make them more stable... Well, ignore my earlier comment. I do like this though and it looks really good. |
@shadowgate15 what's your opinion on adding jsdocs wherever possible? I found it helpful to add inline documentation of some sort and have loose type checking available within IDE. |
I don't really have an opinion on it. Both @niftylettuce and I use vim so type checking doesn't do a whole lot for us. I think that if you want to add it in there and maintain it, then go for it. |
The tests that are failing are only limited to Node v12 and are the same as master's. Failing tests will be resolved through #60 eventually. |
The main aim of this refactor is to simplify extremely complex
validateJob
method and make it easier to reason about job object construction/shape. Initially proposed in this comment.The plan is to:
buildJob
into separate modulethis.config.jobs
validateJob
into separate moduleAbove should not change module's API or change it's behavior.