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

Fix global linting and for in errors #79551

Closed
wants to merge 4 commits into from

Conversation

anthonydresser
Copy link
Contributor

I noticed #79222 was added but the rules weren't actually being applied (apparently because of the casing of the files). There were also some for in errors still in the code.

There were a couple files that linting said shouldn't be accessing some globals but were so I added exceptions for them, not sure if that's actually correct though.

@@ -199,7 +199,7 @@ gulp.task('tslint', () => {
.pipe(filter(tslintExtensionsFilter))
.pipe(gulptslint.default({ rulesDirectory: 'build/lib/tslint' }))
.pipe(gulptslint.default.report({ emitError: true }))
]);
]).pipe(es.through());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this since the gulp task was failing when no errors were being thrown. It still seems to fail correctly when actual errors occur.

@anthonydresser
Copy link
Contributor Author

Some more context on this PR, after finding the tslint was working on window, I found this is a os specific issue. On linux, if I run yarn gulp tslint in master, I get the following warnings

Could not find implementations for the following rules specified in the configuration
       no-nodejs-globals
       no-dom-globals

But on windows works fine. With this branch, both linux and windows work.

@bpasero bpasero requested a review from alexr00 August 22, 2019 06:15
@bpasero bpasero assigned alexr00 and unassigned bpasero Aug 22, 2019
@bpasero bpasero self-requested a review August 22, 2019 06:16
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

Assigning @alexr00 for the task related changes.

I would not change the tslint rules, can you remove that change? I would prefer to disable the tslint at the place where it occurs.

@bpasero
Copy link
Member

bpasero commented Aug 22, 2019

Thanks. I meanwhile pushed a change to disable the rules in code, I guess that is why we see merge conflicts now.

Thanks for the gulp file fix, that I already took over to master 👍

# Conflicts:
#	src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts
#	src/vs/workbench/contrib/tasks/common/tasks.ts
@anthonydresser
Copy link
Contributor Author

@bpasero Sounds good, I'll close this since it seems like its not necessary anymore.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 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.

3 participants