-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fix object in default tag #192
Conversation
Just noticed there's a PR for this already #186 although I guess this one also fixes the inconsistent default spacing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! thanks for opening a PR
packages/util/src/parser.ts
Outdated
@@ -7,7 +7,7 @@ import { | |||
|
|||
export interface IJSDocTag { | |||
tag: string; | |||
value: string; | |||
value: string | Object; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use lowercase object
?
const arr: Array<string | undefined> = [ | ||
fixDescriptionDocblock(description), | ||
...tags.map(({ tag, value }) => | ||
`@${tag} ${typeof value === 'object' ? JSON.stringify(value) : value}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should just JSON.stringify
everything, that way strings are quoted as well. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty indifferent about it. I guess the main disadvantage with it is that it all looks like strings now, even numbers (not counting objects which looks like objects).
w/r/t spacing, it's recommended to run through Prettier (and language-typescript does this by default if installed). keeping track of spacing was a bit of a pain 😄 |
@brettjurgens I've updated the PR with the requested changes as well as commented on the JSON.stringify - check #192 (comment) |
@brettjurgens awesome, can you make a new NPM release as well? |
This PR fixes 2 issues:
Example: In our API we've got something like:
However, using gql2ts breaks when trying to convert that, since
orderBy
is an object. This seems to fix it, although I'm not sure it's the best way to deal with it.