Enforce Dot Notation #47
Conversation
Not sure how strictly we'd like to enforce dot notation for any types
|
I was just writing up an issue to discuss consistency :). This is one that I was going to mention since we chatted about it.
I agree. And, using the |
Great Idea. There are other things on that issue we'd want to address as well. Once its in, can link it here. |
|
Here it is :) - #48 |
* revert changes in lsp due to any type usage * add eslint ignore file * add typescript-eslint rule for dot-notation instead of the vanilla eslint variety
| @@ -0,0 +1 @@ | |||
| .eslintrc.js | |||
There was a problem hiding this comment.
In the eslint extension output log, there were errors in the log about not finding an eslintignore file. The terminal execution looked fine however.
The error was:
Error: ENOENT: no such file or directory, realpath '/Users/jomorr/dev/src/opensource/vscode-sas-extension/.eslintignore'
There was a problem hiding this comment.
I don't see this error on my machine. Do you have specific setting in your extension? I can't think of why this should be an error:) But it's not a big deal. Others good to me
| @@ -4,6 +4,11 @@ module.exports = { | |||
| parserOptions: { | |||
| ecmaVersion: 6, | |||
| sourceType: "module", | |||
| project: [ | |||
There was a problem hiding this comment.
There are more details here: https://eslint.org/docs/latest/user-guide/configuring/configuration-files . Had to look this up because I've never seen it before. I think the last two array entries may be redundant, since the root .tsconfig.json references the client/server specific tsconfig
There was a problem hiding this comment.
This was required due to adding a rule from the typescript-estlint plugin. Without adding the project setting, the following error would be seen:
You have used a rule which requires parserServices to be generated. You must therefore provide a value for the "parserOptions.project" property for @typescript-eslint/parser.
It the client and server tsconfigs are removed, there will be numerous errors to this effect:
0:0 error Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser.
The file does not match your project config: server/src/sas/SyntaxProvider.ts.
The file must be included in at least one of the projects provided
| @@ -2,7 +2,7 @@ | |||
| // Licensed under SAS Code Extension Terms, available at Code_Extension_Agreement.pdf | |||
|
|
|||
| /* eslint-disable @typescript-eslint/explicit-module-boundary-types, | |||
| @typescript-eslint/no-unused-vars,@typescript-eslint/no-explicit-any */ | |||
| @typescript-eslint/no-unused-vars,@typescript-eslint/no-explicit-any,@typescript-eslint/dot-notation */ | |||
There was a problem hiding this comment.
@scnwwu Do you know if this file was copy/pasted from somewhere else? In other words, should it be treated as "generated" code? If not, I'm wondering if there's a plan to cleanup the rules we disable here?
|
I'd like to get @scnwwu's feedback on this one as well, but it looks good to me |
| @@ -0,0 +1 @@ | |||
| .eslintrc.js | |||
There was a problem hiding this comment.
I don't see this error on my machine. Do you have specific setting in your extension? I can't think of why this should be an error:) But it's not a big deal. Others good to me
What should our stance on enforcing dot notation be? I found some spots where javascript key notation is used for getting and setting values. I'd like to propose that we take a stance of using idiomatic typescript whenever possible, including using the dot syntax.
It gets a bit trickier for any types. I'd propose that we leave the enforcement of the rule at error and manually suppress these on a case by case basis. I would think that for this issue, having a lot of manual suppressions would indicate some kind of design smell in the types that we'd want to address.