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

Add setting to show linting errors as warnings #108

Merged
merged 3 commits into from
Nov 29, 2020
Merged

Add setting to show linting errors as warnings #108

merged 3 commits into from
Nov 29, 2020

Conversation

jamesorlakin
Copy link
Contributor

@jamesorlakin jamesorlakin commented May 15, 2020

What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[ ] Bug fix
[x] New feature
[ ] Other, please explain:

On other projects using TSLint I would always turn on the option to display linting failures as warnings, distinguishing a temporary spacing issue from a type mismatch that would cause compilation to fail.

What changes did you make? (Give an overview)
This adds a new (optional) setting to override severity of a Diagnostic so it can appear as a different colour in contrast to severe issues:
image
image
image

Which issue (if any) does this pull request address?
A partial resolve of #95.

Is there anything you'd like reviewers to focus on?
This is my first time touching VS Code extensions - I may have missed something when adding a setting.

Copy link
Member

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

Ohh, that's very neat, I want this! 😄

server/src/server.ts Outdated Show resolved Hide resolved
@theoludwig
Copy link
Contributor

theoludwig commented Nov 17, 2020

Since standard@16.0.1, it introduces "warning" system for disruptive rules (see CHANGELOG.md).

Is it still a viable option, to let people show warning or errors according to their wish ?
In my opinion, we could instead show the real error "state" according to standard rules, what do you think ?

@feross
Copy link
Member

feross commented Nov 17, 2020

@divlo Yeah, I agree. This will be a bit confusing if we treat warnings in our warning system the same as style issues.

In my opinion, we could instead show the real error "state" according to standard rules, what do you think ?

This is already how it works. Warnings related to let and const are shown as warnings and the rest are errors. I'm leaning toward keeping it this way. Thoughts?

@LinusU
Copy link
Member

LinusU commented Nov 18, 2020

Personally, I would still prefer to keep all linting errors as warnings, since that lets me see "compile" errors from TypeScript in red, and linting issues from Standard in yellow

@feross
Copy link
Member

feross commented Nov 18, 2020

Ah, I misunderstood the intent of this PR. I was under the impression that this PR was about distinguishing between standard rules that are stylistic and standard rules that are about correctness.

Am I now correct in my understanding that this PR adds an option that forces all warnings and errors from standard to become warnings? If that is the case, I'm okay with this option.

@theoludwig
Copy link
Contributor

theoludwig commented Nov 19, 2020

"compile" errors from TypeScript in red, and linting issues from Standard in yellow

Right, seperating lint errors to compile errors of TypeScript is great, good point! @LinusU

this PR adds an option that forces all warnings and errors from standard to become warnings

Yes, it is correct!

I'm okay with this option if it stays disabled by default but it is the case, so alright for me.
But badly there are conflicts with master, could you try to solve them ? @jamesorlakin
Thanks for your contribution!

When conflicts are solved, we will review again and we could probably merge that to master.

@jamesorlakin
Copy link
Contributor Author

jamesorlakin commented Nov 29, 2020

Hi folks,

Indeedy, I realise my branch name is actually the opposite of what I meant - oops! Anyhow, this is a just a purely visual change inspired by personal preference. I think this all predates Standard having a difference between warnings and errors itself, but at the end of the day it's all about code style vs code errors.

For clarity, here's a 'off and on' view of the same code with 'Check JS' switched on:
image

image

I've just manually recreated the changes on master.

Thanks.

@theoludwig
Copy link
Contributor

theoludwig commented Nov 29, 2020

For some reason, it seems like it doesn't work with ts-standard :

On the master branch (as you can see the opacity of "standard.treatErrorsAsWarnings": true) :
Capture1

On this PR branch :
Capture1_1

I don't see the error about the semicolon for example by ts-standard. I tested multiple rules but still I've got no errors at all.

Could you test it with ts-standard, please ? @jamesorlakin
If you can get it to work, share it with us. 👍
Also we should add this new setting inside README.

@jamesorlakin
Copy link
Contributor Author

Hmm, for some reason I can't get any output on master at all. I've cleaned both the out folders as well.

image

@theoludwig
Copy link
Contributor

theoludwig commented Nov 29, 2020

@jamesorlakin Yes, my bad. Actually this PR #140 broke ts-standard because it couldn't find tsconfig.json but now with #139, I make it so it only works with standard engine not the others one, so everything is working as expected.
I'll merge that PR to master! 👍
Once this has been merged could you test the version on master and tell us if it's working or not in #135 so we can release v1.5.0 ?
Thanks! 😄

@theoludwig theoludwig merged commit 0f92bb6 into standard:master Nov 29, 2020
@welcome
Copy link

welcome bot commented Nov 29, 2020

🎉 Congrats on getting your first pull request landed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants