n3ps / json-schema-to-jsdoc

Generate JSDoc from JSON Schema
https://francisn.com/json-schema-to-jsdoc/
ISC License
30 stars 11 forks source link

Property types #28

Closed brettz9 closed 4 years ago

brettz9 commented 4 years ago
n3ps commented 4 years ago

For the option name, I think defaultType or defaultPropertyType are clearer. Also if it's string|boolean, what happens when it's true?

And possibly propertyNameAsType for the other one. Btw, default value false not mentioned in Readme

brettz9 commented 4 years ago

For the option name, I think defaultType or defaultPropertyType are clearer. ... And possibly propertyNameAsType for the other one.

defaultPropertyType and propertyNameAsType sound good to me.

Also if it's string|boolean, what happens when it's true?

Good point. How about I change boolean to just the literal false in the signature (unless you want true to mean our own default (of *) should be used--see below).

Btw, default value false not mentioned in Readme

Sorry, I thought you changed to want * as the default. Do you want me to switch to false as the default (in which case the user will have to supply * themselves)?

n3ps commented 4 years ago

Ah for the readme what I meant was that the default (whether true/false) wasn't mentioned.

How about defaultPropertyType as simply string, with '' (empty string) as the special case of "no types added"?

n3ps commented 4 years ago

Travis says all current PRs are green.

I can relate. I was in Hangzhou last year and had to deal with VPN issues throughout.the trip

On Sun, Jul 5, 2020 at 12:40 AM Brett Zamir notifications@github.com wrote:

I'm afraid I am not able to get over the GFW to see why the Travis build is failing. :-( It is working for me locally. Would you mind reporting back the error?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/n3ps/json-schema-to-jsdoc/pull/28#issuecomment-653841312, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAINVTR4P5HEGOFXEVKE7LDRZ775FANCNFSM4OQGV6NQ .

brettz9 commented 4 years ago

I thought about that, but my thinking was that the empty string might be used so that it could be left empty to be filled in later. Maybe null would be better, as it is not binary? But if you want the empty string, I can do that.

n3ps commented 4 years ago

Ok null should work. In the docs, the default will then be undefined

On Sun., Jul. 5, 2020, 3:20 a.m. Brett Zamir notifications@github.com wrote:

I thought about that, but my thinking was that the empty string might be used so that it could be left empty to be filled in later. Maybe null would be better, as it is not binary? But if you want the empty string, I can do that.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/n3ps/json-schema-to-jsdoc/pull/28#issuecomment-653852164, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAINVTVAJWME6KFNUAIP263R2ASUXANCNFSM4OQGV6NQ .

brettz9 commented 4 years ago

I guess you mean defaulting to undefined, i.e., it defaults to *? If that was the intent, I thought it would be clearer to say that * was the default (where default implies the value was undefined but we clarify to the user that * is what will end up getting used in such a case).

If you instead mean you want undefined to behave like null, i.e., adding no brackets or type, let me know, and I can change it.

I also changed types to expect null instead of false both because it does not accept true and because it provides a similar behavior of avoiding brackets entirely (though on @typedef instead of @property).

I also added mention of two booleans defaults (being false) as noticed these had not been added.

n3ps commented 4 years ago

Ah right, I had thought this was one of those "unset" options, which will eventually result to the * fallback.

Setting the default to * is more direct.