vscode-highlight-bad-chars
vscode-highlight-bad-chars copied to clipboard
WengerK
feature: add bad characters to the problems pane. closes #2
This PR adds the bad characters to the problem pane. I hope you'll find it useful. There are some bugs that I don't have the time to fix right now but I'll try to update this PR quickly:
- [x] the problems are not removed when the editor is closed
- [x] you cannot set the severity of the problems
- [x] the name of the extension is not shown next to the problem
Consider this a WIP and contribute to it if you want to speed it up!
Hey Many Many thanks for you awesome contrib ! I'm waiting on #22 to be merged before, if it's okay for you ?!
We also should consider adding tests for the problems pane :)
we recently had a "major" regression with all tabs to be highlighted :) I'd like to have a covering of feature to prevent futur issue of this kind ^^'
It's perfect to me, I was just making sure you were aware of this PR :+1:. I'll don't have much experience with Jest but I may give it a try and add test cases, however, I'm pretty busy right now so I can't promise to get it done quickly.
Have a nice day and thanks for the response!
@WengerK, I'm having a problem passing the tests you wrote, it always fails with:
TypeError: Cannot read property 'Error' of undefined
38 | break;
39 | default:
> 40 | errorSeverity = vscode.DiagnosticSeverity.Error;
| ^
41 | break;
42 | }
43 |
at loadConfiguration (src/extension.ts:40:55)
at activate (src/extension.ts:64:18)
at Object.<anonymous> (test/extension.test.ts:168:9)
I tried adding the following lines with no luck so far:
diff --git a/test/extension.test.ts b/test/extension.test.ts
index 3f6f533..e3788aa 100644
--- a/test/extension.test.ts
+++ b/test/extension.test.ts
@@ -1,5 +1,5 @@
-import type {DecorationRenderOptions} from 'vscode';
-import {contributes} from '../package.json';
+import type { DecorationRenderOptions, DiagnosticSeverity } from 'vscode';
+import { contributes } from '../package.json';
const configDefinition = contributes.configuration;
export type ExtensionConfig = {
@@ -7,6 +7,7 @@ export type ExtensionConfig = {
additionalUnicodeChars?: string[],
allowedUnicodeChars?: string[],
asciiOnly?: boolean,
+ errorSeverity?: DiagnosticSeverity,
};
/**
@@ -129,12 +130,14 @@ beforeEach(() => {
const allowedUnicodeChars = configDefinition.properties['highlight-bad-chars.allowedUnicodeChars'].default;
const badCharDecorationStyle = configDefinition.properties['highlight-bad-chars.badCharDecorationStyle'].default;
const asciiOnly = configDefinition.properties['highlight-bad-chars.asciiOnly'].default;
+ const errorSeverity = configDefinition.properties['highlight-bad-chars.severity'].default;
mockConfiguration = {
additionalUnicodeChars,
allowedUnicodeChars,
badCharDecorationStyle,
asciiOnly,
+ errorSeverity,
};
});
diff --git a/src/extension.ts b/src/extension.ts
index 29205d5..453c74a 100644
--- a/src/extension.ts
+++ b/src/extension.ts
@@ -8,6 +8,7 @@ export type ExtensionConfig = {
additionalUnicodeChars?: string[],
allowedUnicodeChars?: string[],
asciiOnly?: boolean,
+ errorSeverity?: vscode.DiagnosticSeverity,
};
function loadConfiguration(): {
Could you give me a hand?
@WengerK, I'm having a problem passing the tests you wrote, it always fails with:
TypeError: Cannot read property 'Error' of undefined 38 | break; 39 | default: > 40 | errorSeverity = vscode.DiagnosticSeverity.Error; | ^ 41 | break; 42 | } 43 | at loadConfiguration (src/extension.ts:40:55) at activate (src/extension.ts:64:18) at Object.<anonymous> (test/extension.test.ts:168:9)I tried adding the following lines with no luck so far:
diff --git a/test/extension.test.ts b/test/extension.test.ts index 3f6f533..e3788aa 100644 --- a/test/extension.test.ts +++ b/test/extension.test.ts @@ -1,5 +1,5 @@ -import type {DecorationRenderOptions} from 'vscode'; -import {contributes} from '../package.json'; +import type { DecorationRenderOptions, DiagnosticSeverity } from 'vscode'; +import { contributes } from '../package.json'; const configDefinition = contributes.configuration; export type ExtensionConfig = { @@ -7,6 +7,7 @@ export type ExtensionConfig = { additionalUnicodeChars?: string[], allowedUnicodeChars?: string[], asciiOnly?: boolean, + errorSeverity?: DiagnosticSeverity, }; /** @@ -129,12 +130,14 @@ beforeEach(() => { const allowedUnicodeChars = configDefinition.properties['highlight-bad-chars.allowedUnicodeChars'].default; const badCharDecorationStyle = configDefinition.properties['highlight-bad-chars.badCharDecorationStyle'].default; const asciiOnly = configDefinition.properties['highlight-bad-chars.asciiOnly'].default; + const errorSeverity = configDefinition.properties['highlight-bad-chars.severity'].default; mockConfiguration = { additionalUnicodeChars, allowedUnicodeChars, badCharDecorationStyle, asciiOnly, + errorSeverity, }; });diff --git a/src/extension.ts b/src/extension.ts index 29205d5..453c74a 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -8,6 +8,7 @@ export type ExtensionConfig = { additionalUnicodeChars?: string[], allowedUnicodeChars?: string[], asciiOnly?: boolean, + errorSeverity?: vscode.DiagnosticSeverity, }; function loadConfiguration(): {Could you give me a hand?
I'll have some spare.time next week, I'll take a look
coverage added @puka-tchou. I let you re-ping me whenever the PR is ready for testing/merging.
It is, feel free to review and merge whenever you feel like it @WengerK (and @ItalyPaleAle too I guess) :fireworks:
I'd like to have the review of @ItalyPaleAle before merging :)
Many thanks for your contrib by the way 🚀
Thanks again for doing this @puka-tchou! I think this PR looks great and I'm fine with it being merged.
The only question that comes to me now, and it's more of a question for @WengerK , is if at this point we should stop using decorators and just rely on the problem reporting?
Example:
-
With decorators on (what's currently happening today, notice both the red square and the wiggly underline):
-
Without decorators:
What do you think?
It's a good point. I don't know.
What about having a toggle for both feature ?
I think that's a good idea. My vote (but it's your extension so you have final say) is on keeping them both on by default but making the red squares something that can be disabled.
This way we introduce the new, improved format but also maintain consistency for existing users.
I agree. I'm in favor of having gracefull enhancment, for existint user toggler for the New feature goes in this direction.
@puka-tchou would you add this new config (with readme changes) ?
I do agree but I think it's beyond the scope of this PR. Adding this new configuration option means changing the README, the code and the tests to make sure everything is OK. I'll rather make another PR once this one has been merged. Is that OK with you @WengerK ?
Actually, as I worked on #29, I want to take back my comments. I don't think we should offer an option to disable the red boxes.
When there are zero-width characters, having the red boxes (lines, in this case) is much better:
Well, if everything is OK for you, I would love to finally merge this :)
I'll work on it tuesday. I think we should:
- edit the readme with new config
- add new printscreen with feature
- allow toggle feature of problem pane
I'll work on it tuesday. I think we should:
- edit the readme with new config
- add new printscreen with feature
- allow toggle feature of problem pane
I did not get the chance to work on it unfortunatelly, critical issues got priorities over this.
Hey @WengerK, I just updated the README. The image is broken for now but it will work once merged. I don't feel the need to allow to enable or disable the problems pane feature as you can filter what you see in the problem pane :
Here is the image that I added in the README:
I am starting to feel that this PR is taking too much time and I fear that I'm gonna lose interest in keeping it up-to-date, so the faster it's merged, the better. However, this is your repo and your call, so feel free to keep updating the PR!
You can fix images by making all links relative (which is actually better). For example, from:
To:
