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

Revisit use of typedef linting rule #10573

Open
csnover opened this issue Feb 22, 2023 · 4 comments
Open

Revisit use of typedef linting rule #10573

csnover opened this issue Feb 22, 2023 · 4 comments

Comments

@csnover
Copy link

csnover commented Feb 22, 2023

PR #10559 fails due to linter errors about a lint that probably should not be enabled.

The use of typedef lint was added to vscode-cpptools in #1403 with the comment “explicit types is recommended Typescript practice”, but this is not correct. Actually, the documentation for this lint explicitly states the opposite:

Requiring type annotations unnecessarily can be cumbersome to maintain and generally reduces code readability. TypeScript is often better at inferring types than easily written type annotations would allow.

Instead of enabling typedef, it is generally recommended to use the --noImplicitAny and --strictPropertyInitialization compiler options to enforce type annotations only when useful.

The compiler is already using the given flags as strict: true is set, so I would recommend to remove this lint as it adds line noise and runs against best practices.

Let me know your thoughts. Thanks!

@sean-mcmanus
Copy link
Contributor

I'm fine with changing the linting rule, but you should wait to see what others think. @WardenGnaw @bobbrow @Colengms @michelleangela

@bobbrow
Copy link
Member

bobbrow commented Feb 22, 2023

I'm ok to change the rule. CMake Tools doesn't have it, and I think it's nice not to have to declare types in some cases where it is obvious what it is.

@sean-mcmanus
Copy link
Contributor

Yeah, similar to "auto" usage with C++.

@csnover
Copy link
Author

csnover commented Mar 1, 2023

Is there any other feedback on this? Otherwise it sounds like the change should be made and I will open another PR for that. Thanks!

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

No branches or pull requests

3 participants