Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Make JSDoc skipping public, plus more configurable #55739
Make JSDoc skipping public, plus more configurable #55739
Changes from 16 commits
442b467
9119d2f
ba9dcab
8a4837c
66a0b09
46bbf0e
88bfd9e
2abd21b
b691830
9e2f8c1
3d60c92
fb22a3e
626ae3e
b6379cb
a273a91
a430b30
3ebdfb6
3673f9e
f393631
40b0cc4
12f5903
735c17c
b3e23a9
db14c33
517f289
9392280
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 dont think we should be threading in this to defaults. User of this API can always call thius method and override getSourceFile on their own .. there is nothing special here.
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 that not providing this parameter will make it very inconvenient; this function and
createCompilerHostWorker
are what callcreateGetSourceFile
, which is internal and non-trivial, and then wrap it again in another closure. Setting the JSDoc parsing mode is a lot like setting a defaultsetParentNodes
which this function already does.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.
See: https://github.com/typescript-eslint/typescript-eslint/blob/7826910d8c6ba4d2d40b8d55fcf7fd39fe290d88/packages/typescript-estree/src/create-program/useProvidedPrograms.ts#L84
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.
Generally speaking I think this means that anywhere we can expect to find
setParentNodes
, we probably need to be expectingjsDocParsingMode
.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.
Hmm.. Generally i like to go with conservative approach and avoid having to add additional surface area that we have to keep maintaining. This is ok i guess though , user can just create source file from file text easily to override but ok.
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.
If the option is fed in via the
CreateSourceFileOptions
, rather than a top-level field, callers will already have the opportunity to request it on a per-file basis when making the file, rather than needing to set it on the host as a whole.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.
👍🏼
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.
ScriptKind.JSON
? AFAIK none of the comments we may read in a JSON document can modify the expression types, but we might actually accidentally allow it.ScriptKind.External
is probably also safe to bail on.ScriptKind.Deferred
I don't really know.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 don't think we ever actually make it to this check at all for those file types, so I felt safe not checking for them (and choosing the backwards-compatible option). But, I'll verify.
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.
Flipping this to be "=== JS, === JSX" doesn't change any tests, but truthfully I don't know when the others are even used. I would feel safest with the conservative I have now, but am not strongly attached if someone knows better.