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

Patch/#159 default tag bug #160

Merged
merged 25 commits into from
Apr 10, 2022

Conversation

Dan503
Copy link
Contributor

@Dan503 Dan503 commented Apr 4, 2022

This fixes issue #159

src/parser.ts Outdated Show resolved Hide resolved
tests/default.test.ts Show resolved Hide resolved
@Dan503
Copy link
Contributor Author

Dan503 commented Apr 9, 2022

FYI, I tried implementing the solution they gave in the answer to this open issue:
syavorsky/comment-parser#155

It didn't work. It also broke a bunch of other tests so I'm going to steer away from it.

@Dan503

This comment was marked as resolved.

@hosseinmd
Copy link
Owner

Seem option 2 is better, prettier default using double quotes, Unless singleQuote defined by the user in prettier options

Daniel Tonon added 8 commits April 10, 2022 10:32
…alue

It is being too troublesome to implement.
It also isn't part of the official jsDoc spec.
I think people would most likely either use values directly or use the JS doc official syntax so I don't see much reason in trying to support it.
I'm not trying to match against both square and curly boilerplates so I only need the one boiler plate type to match against
@Dan503
Copy link
Contributor Author

Dan503 commented Apr 10, 2022

@hosseinmd can you take another look?

I've resolved all feedback.

I decided it wasn't worth trying to support @default {{ object: 'value' }} with the { ... } as boilerplate wrapped around the value.

The official jsDoc spec says that if there is going to be boilerplate wrapped around the value then it would be @default [{ object: 'value' }] with [ ... ] being the boilerplate.

So with this PR I'm supporting both of these formats:

  • @default value - a lean version that VS Code supports
  • @default [value] - official jsDoc format with [ ... ] as boilerplate around the default value

I'm not supporting @default {value} though since it's difficult to support and there isn't a strong enough use case behind it.

UPDATE

@default [value] support has been dropped since the documentation just means the value is optional. It doesn't mean you are supposed to wrap the value in square brackets.

Copy link
Owner

@hosseinmd hosseinmd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about [{}]

jsdoc documented this @default [<some value>] but it doesn't mean to add type inside []

src/utils.ts Show resolved Hide resolved
@Dan503

This comment was marked as resolved.

src/utils.ts Show resolved Hide resolved
tests/__snapshots__/default.test.ts.snap Outdated Show resolved Hide resolved
Daniel Tonon added 3 commits April 10, 2022 21:23
That was a misunderstanding. The documentation means that the value is optional. It doesn't want you to put litteral square brackets in your code.
Daniel Tonon added 4 commits April 10, 2022 21:56
The official documentation says that `@defaultvalue` should be all lowercase.

Implementing support for this was more difficult than I expected. I'm droping this as being out of scope for this PR.

I have no intention of using the all lowercase `@defaultvalue` tag so I don't care about trying to support it.
@Dan503
Copy link
Contributor Author

Dan503 commented Apr 10, 2022

@hosseinmd ready for another review.

I removed support for the @default [value] format. It only supports the basic @default value syntax now.

I also attempted to implement support for @defaultvalue (all lowercase) but it was being too problematic so I'm dropping it as being out of scope for this PR.

@hosseinmd hosseinmd merged commit 11ef0c5 into hosseinmd:master Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants