-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Finish applying no-object-literal-type-assertion
lint rule
#18218
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
Conversation
7553b3b
to
8745182
Compare
d2b5e35
to
6ccb5e7
Compare
projectName: req.projectName | ||
}); | ||
projectName: req.projectName, | ||
// TODO: GH#18217 (property was not present) |
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.
why is this a problem?
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 was just wondering if maybe it should be present. We never declared this property to be optional.
src/server/project.ts
Outdated
@@ -1215,6 +1215,7 @@ namespace ts.server { | |||
|
|||
// Provide global: true so plugins can detect why they can't find their config | |||
this.projectService.logger.info(`Loading global plugin ${globalPluginName}`); | |||
// tslint:disable-next-line no-object-literal-type-assertion (TODO: GH#18217) |
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 like we should change the interface definition here instead.
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.
But the fact that the interface doesn't declare it would imply that we're not using the property?
src/server/editorServices.ts
Outdated
@@ -1328,6 +1328,7 @@ namespace ts.server { | |||
|
|||
const result = parseJsonText(configFilename, configFileContent); | |||
if (!result.endOfFileToken) { | |||
// tslint:disable-next-line no-object-literal-type-assertion (TODO: GH#18217) | |||
result.endOfFileToken = <EndOfFileToken>{ kind: SyntaxKind.EndOfFileToken }; |
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.
why do we do this in the first place.. @sheetalkamat any ideas?
@@ -914,6 +914,7 @@ namespace ts { | |||
*/ | |||
export function readJsonConfigFile(fileName: string, readFile: (path: string) => string | undefined): JsonSourceFile { | |||
const textOrDiagnostic = tryReadFile(fileName, readFile); | |||
// tslint:disable-next-line no-object-literal-type-assertion (TODO:GH#18217) |
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.
we should change the return type for this function instead.
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.
To { parseDiagnostics: Diagnostic[] }
? That exposes errors in its uses because we seem to assume we get the full source file.
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.
with optional members for fileName
and extendedSourceFiles
.. not sure if we need other things as well.
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.
that might end up touching many places though..
Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it. |
6da37ac
to
5d86382
Compare
5d86382
to
6de3bde
Compare
Some of these are OK but I feel like overall this is adding too many disables to look like a good fit |
Continuation of #17278.
Applies the rule wherever possible and adds comments referencing #18217 at all fishy code.