Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Add more linting.#3847

Merged
sean-mcmanus merged 7 commits into
mastermicrosoft/vscode-cpptools:masterfrom
seanmcm/addMoreLintingmicrosoft/vscode-cpptools:seanmcm/addMoreLintingCopy head branch name to clipboard
Jul 5, 2019
Merged

Add more linting.#3847
sean-mcmanus merged 7 commits into
mastermicrosoft/vscode-cpptools:masterfrom
seanmcm/addMoreLintingmicrosoft/vscode-cpptools:seanmcm/addMoreLintingCopy head branch name to clipboard

Conversation

@sean-mcmanus

@sean-mcmanus sean-mcmanus commented Jun 27, 2019

Copy link
Copy Markdown
Contributor

I hit a bug in my FindAllReferences branch that would have been caught by more linting. Most of the changes are whitespace, or other trivial/quick changes.

@sean-mcmanus sean-mcmanus requested a review from a team June 27, 2019 22:19
Comment thread Extension/tslint.json Outdated
@pieandcakes

pieandcakes commented Jun 27, 2019

Copy link
Copy Markdown
Contributor

@sean-mcmanus Why are you disabling a lot of items in tslint when you say you are "Adding more linting"? I think you need to explain more about why certain things are being turned off. Like variable-name and array-type are not trivial and should be enforced.

@WardenGnaw WardenGnaw left a comment

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.

There is a lot of lint changes that are arguably should be enabled.

@sean-mcmanus

Copy link
Copy Markdown
Contributor Author

There is a lot of lint changes that are arguably should be enabled.

Yeah, but we were failing so I disabled them for now, but they're listed as "false" so they're easier to discover. If there are any in particular, you think we should try to fix now, I could investigate more.

@sean-mcmanus

sean-mcmanus commented Jun 28, 2019

Copy link
Copy Markdown
Contributor Author

@sean-mcmanus Why are you disabling a lot of items when you say you are "Adding more linting"

Did I disable existing linting errors? I don't see any. The intention was to explicitly add the things that are disabled so we could revisit them later.

@sean-mcmanus sean-mcmanus added this to the July 2019 milestone Jun 28, 2019
Comment thread Extension/tslint.json Outdated
Comment thread Extension/tslint.json Outdated
Comment thread Extension/tslint.json Outdated
Comment thread Extension/tslint.json Outdated
Comment thread Extension/tslint.json Outdated
Comment thread Extension/tslint.json Outdated
Comment thread Extension/tslint.json Outdated
Comment thread Extension/tslint.json Outdated
Comment thread Extension/tslint.json Outdated
Comment thread Extension/tslint.json Outdated
@sean-mcmanus

Copy link
Copy Markdown
Contributor Author

@sean-mcmanus Why are you disabling a lot of items in tslint when you say you are "Adding more linting"? I think you need to explain more about why certain things are being turned off. Like variable-name and array-type are not trivial and should be enforced.

I think I fixed those now.

@sean-mcmanus

Copy link
Copy Markdown
Contributor Author

There is a lot of lint changes that are arguably should be enabled.

I added the TODO's to our internal docs and removed the "false" rules.

Comment thread Extension/tslint.json
"prefer-while": true,
"promise-must-complete": true,
"semicolon": true,
"space-within-parens": true,

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, this is correct. The docs are wrong. Or...it's correct with my version of TS Lint, which enforces the 0 space rule when "true" is used and doesn't accept a number like the docs say.

Comment thread Extension/tslint.json
@sean-mcmanus sean-mcmanus merged commit ccc18a6 into master Jul 5, 2019
@sean-mcmanus sean-mcmanus deleted the seanmcm/addMoreLinting branch July 5, 2019 21:39
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 8, 2020
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.

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.