Skip to content

getTokenAtPosition: default includeJsDocComment to true#25015

Merged
7 commits merged into
masterfrom
getTokenAtPosition
Jun 26, 2018
Merged

getTokenAtPosition: default includeJsDocComment to true#25015
7 commits merged into
masterfrom
getTokenAtPosition

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Jun 16, 2018

Sequel to #23379. Would have made #25014 unnecessary.

@ghost ghost requested a review from sheetalkamat June 16, 2018 00:54
@ghost ghost force-pushed the getTokenAtPosition branch from f451235 to 35da4ae Compare June 18, 2018 16:07
Comment thread src/services/utilities.ts Outdated
/** Returns a token if position is in [start-of-leading-trivia, end) */
export function getTokenAtPosition(sourceFile: SourceFile, position: number, includeJsDocComment: boolean, includeEndPosition = false): Node {
return getTokenAtPositionWorker(sourceFile, position, /*allowPositionInLeadingTrivia*/ true, /*includePrecedingTokenAtEndPosition*/ undefined, includeEndPosition, includeJsDocComment);
export function getTokenAtPosition(sourceFile: SourceFile, position: number, includeJsDocComment = true): Node {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From the change it looks like there is no place remaining where we need includeJsDocComment as false? Why have a parameter then. If this is rare scenario may be change the parameter to take optional value without assigning default and do !!ignoreJsDocComment so that we save assigning default value to the parameter

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

There are a few left actually, though they're suspicious. #25162

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If that's the case I wouldn't make this parameter optional (could be done in #25612) but here we should keep the comments and pass true I think. Making it optional makes it more confusing.

@ghost
Copy link
Copy Markdown
Author

ghost commented Jun 22, 2018

Latest commit fixes #25162

@ghost
Copy link
Copy Markdown
Author

ghost commented Jun 26, 2018

@sheetalkamat Please re-review

{ marker: "lessInComment", exact: undefined, triggerCharacter: "<" },

{ marker: "openTag", includes: "div", triggerCharacter: "<" },
{ marker: "openTag", exact: undefined, triggerCharacter: "<" }, // TODO: `includes: "div"`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is there issue opened for this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed by #25167

@ghost ghost merged commit 13bc46d into master Jun 26, 2018
@ghost ghost deleted the getTokenAtPosition branch June 26, 2018 23:20
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants