Fix MakeDirectoryOptions for node 12#43352
Conversation
|
@simhnna Thank you for submitting this PR! 🔔 @microsoft @DefinitelyTyped @jkomyno @a-tarasyuk @alvis @r3nya @btoueg @BrunoScheufler @smac89 @tellnes @Touffy @DeividasBakanas @eyqs @Flarna @Hannes-Magnusson-CK @KSXGitHub @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @n-e @octo-sniffle @galkin @parambirs @eps1lon @SimonSchick @ThomasdenH @WilcoBakker @wwwy3y3 @samuela @kuehlein @j-oliveras @bhongy @chyzwar @trivikr @nguymin4 @yoursunny @qwelias @ExE-Boss @Ryan-Willpower @peterblazejewicz @ZaneHannanAU @jeremiergz @ivansieder - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
|
@simhnna - It appears Travis did not correctly run on this PR! /cc @RyanCavanaugh to investigate and advise. |
Flarna
left a comment
There was a problem hiding this comment.
Please update also /types/node/fs.d.ts
ExE-Boss
left a comment
There was a problem hiding this comment.
There should probably be a documentation on the expected string format somewhere.
|
I found out node 11 types were also wrong. The format isn't documented in the nodejs api docs. It only mentions the default value |
|
@simhnna One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you! |
Flarna
left a comment
There was a problem hiding this comment.
Usually there is no need to adapt EOL versions like node 11 but it's not wrong either.
|
@simhnna The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
|
I'm not sure why the tests are failing. It doesn't appear to be anything related to my changes. Anyone got any ideas? |
|
I think there was some issue because 3.9. Maybe it was fixed meanhwhile. Try to rebase or merge master. This should retrigger CI. |
You should be able to run local npm test now to verify status and push update (or push empty commit) to restart CI: |
|
@simhnna I haven't seen anything from you in a while and this PR currently has problems that prevent it from being merged. The PR will be closed tomorrow if there aren't new commits to fix the issues. |
|
A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped! |
|
Congratulations on your first DefinitelyTyped contribution! |
|
I just published |
Please fill in this template.
npm test.)npm run lint package-name(ortscif notslint.jsonis present).If changing an existing definition:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }. If for reason the any rule need to be disabled, disable it for that line using// tslint:disable-next-line [ruleName]and not for whole package so that the need for disabling can be reviewed.