feat: add writeJson/writeJsonSync for fs modules #271
Conversation
fs/write_json.ts
Outdated
| * @param {string} filePath | ||
| * @param {*} object | ||
| * @param {WriteJsonOption} [options] | ||
| * @returns {Promise<void>} |
There was a problem hiding this comment.
Aren't these docs superfluous given type annotations? CC @kitsonk
There was a problem hiding this comment.
Correct. We shouldn't be using JSDoc like this.
There was a problem hiding this comment.
Without revelant descriptions of parameters?
There was a problem hiding this comment.
If you want to provide descriptions you can do that, this JSDoc is not doing that. If providing descriptions is informative then the following would be acceptable:
/** Some description
* @param filePath The path to the file
*/What is included here is not acceptable. Need to find out if there is an eslint rule for this.
There was a problem hiding this comment.
you mean this one: https://eslint.org/docs/rules/valid-jsdoc ?
There was a problem hiding this comment.
No, because those include the types... we want something that does not include the types.
@j-f1 any rule you know of to keep types out of JSDoc?
There was a problem hiding this comment.
@kitsonk you may look this option: https://eslint.org/docs/rules/valid-jsdoc#requireparamtype . If set to false it fits your need.
There was a problem hiding this comment.
Oh right. But adding enforce of JsDoc good atm? And when this feature will be integrated we add this config to eslint no?
There was a problem hiding this comment.
I think this interface is obvious and there is no need to explain the parameters.
So I only leave a single line comment
Co-Authored-By: axetroy <troy450409405@gmail.com>
| @@ -0,0 +1,57 @@ | |||
| // Copyright 2018-2019 the Deno authors. All rights reserved. MIT license. | |||
| /* eslint-disable @typescript-eslint/no-explicit-any */ | |||
There was a problem hiding this comment.
Should we ignore any warning?
I didn't find the answer in the style guide.
But it seems that the other modules have not ignored it.
There was a problem hiding this comment.
I've just fixed the date module. But we may not have any warning. I'll check on the flag module to fix it.
part of #261