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

feat: add writeJson/writeJsonSync for fs modules #271

Merged
merged 5 commits into from Mar 14, 2019

Conversation

6 participants
@axetroy
Copy link
Contributor

axetroy commented Mar 13, 2019

part of #261

@axetroy axetroy referenced this pull request Mar 13, 2019

Open

implement fs-extra api for fs modules #261

7 of 13 tasks complete
* @param {string} filePath
* @param {*} object
* @param {WriteJsonOption} [options]
* @returns {Promise<void>}

This comment has been minimized.

@bartlomieju

bartlomieju Mar 13, 2019

Contributor

Aren't these docs superfluous given type annotations? CC @kitsonk

This comment has been minimized.

@kitsonk

kitsonk Mar 13, 2019

Contributor

Correct. We shouldn't be using JSDoc like this.

This comment has been minimized.

@zekth

zekth Mar 13, 2019

Contributor

Without revelant descriptions of parameters?

This comment has been minimized.

@kitsonk

kitsonk Mar 13, 2019

Contributor

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.

This comment has been minimized.

@zekth

This comment has been minimized.

@kitsonk

kitsonk Mar 13, 2019

Contributor

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?

This comment has been minimized.

@zekth

zekth Mar 13, 2019

Contributor

@kitsonk you may look this option: https://eslint.org/docs/rules/valid-jsdoc#requireparamtype . If set to false it fits your need.

This comment has been minimized.

This comment has been minimized.

@zekth

zekth Mar 13, 2019

Contributor

Oh right. But adding enforce of JsDoc good atm? And when this feature will be integrated we add this config to eslint no?

This comment has been minimized.

@axetroy

axetroy Mar 14, 2019

Author Contributor

I think this interface is obvious and there is no need to explain the parameters.
So I only leave a single line comment

Show resolved Hide resolved fs/write_json.ts Outdated
Show resolved Hide resolved fs/write_json.ts Outdated
Show resolved Hide resolved fs/write_json.ts Outdated
Show resolved Hide resolved fs/write_json.ts Outdated
Show resolved Hide resolved fs/write_json.ts Outdated

kitsonk and others added some commits Mar 14, 2019

Rename WriteJsonOption to WriteJsonOptions
Co-Authored-By: axetroy <troy450409405@gmail.com>

This was referenced Mar 14, 2019

@@ -0,0 +1,57 @@
// Copyright 2018-2019 the Deno authors. All rights reserved. MIT license.
/* eslint-disable @typescript-eslint/no-explicit-any */

This comment has been minimized.

@axetroy

axetroy Mar 14, 2019

Author Contributor

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.

This comment has been minimized.

@zekth

zekth Mar 14, 2019

Contributor

I've just fixed the date module. But we may not have any warning. I'll check on the flag module to fix it.

@ry

ry approved these changes Mar 14, 2019

Copy link
Contributor

ry left a comment

LGTM

@ry ry merged commit e9d104a into denoland:master Mar 14, 2019

2 checks passed

denoland.deno_std #20190314.5 succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.