feature: add bad characters to the problems pane. closes #2 #27

puka-tchou · 2021-10-07T11:53:12Z

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:

the problems are not removed when the editor is closed
you cannot set the severity of the problems
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!

puka-tchou · 2021-10-11T06:58:37Z

@WengerK, if you have the chance to review this PR it would be nice!

WengerK · 2021-10-11T07:04:43Z

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 ^^'

puka-tchou · 2021-10-12T07:08:03Z

It's perfect to me, I was just making sure you were aware of this PR 👍.
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!

ItalyPaleAle

Thank you for doing this! It's a great PR, just a couple of minor changes :)

package.json

src/extension.ts

WengerK · 2021-10-19T07:50:23Z

back merge master here to run tests & linter

puka-tchou · 2021-10-20T13:39:47Z

@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 · 2021-10-25T16:29:02Z

@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

WengerK · 2021-10-26T08:10:56Z

coverage added @puka-tchou. I let you re-ping me whenever the PR is ready for testing/merging.

puka-tchou · 2021-10-26T09:24:03Z

It is, feel free to review and merge whenever you feel like it @WengerK (and @ItalyPaleAle too I guess) 🎆

test/extension.test.ts

WengerK · 2021-10-26T10:16:12Z

I'd like to have the review of @ItalyPaleAle before merging :)

Many thanks for your contrib by the way 🚀

ItalyPaleAle · 2021-10-26T17:39:14Z

Hey folks, I'm on vacation this week. Will take a look next week!

ItalyPaleAle · 2021-11-01T02:10:12Z

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?

WengerK · 2021-11-01T05:27:55Z

It's a good point. I don't know.

What about having a toggle for both feature ?

ItalyPaleAle · 2021-11-01T05:33:44Z

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.

WengerK · 2021-11-01T05:50:41Z

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) ?

puka-tchou · 2021-11-01T10:59:00Z

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 ?

ItalyPaleAle · 2021-11-01T20:25:14Z

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:

puka-tchou · 2021-11-02T12:06:36Z

Well, if everything is OK for you, I would love to finally merge this :)

WengerK · 2021-11-02T12:18:04Z

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

WengerK · 2021-11-04T16:45:03Z

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.

puka-tchou · 2021-11-12T09:04:25Z

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!

ItalyPaleAle · 2021-11-12T18:06:50Z

You can fix images by making all links relative (which is actually better). For example, from:

![highlight-bad-chars-configuration](https://raw.githubusercontent.com/WengerK/vscode-highlight-bad-chars/master/images/highlight-bad-chars-configuration.settings.png)

To:

![highlight-bad-chars-configuration](/images/highlight-bad-chars-configuration.settings.png)

puka added 4 commits October 7, 2021 13:31

feature: add bad characters to the problems pane. closes WengerK#2

5a6cdf3

feat: adds the ability to configure the severity

ce46126

fix: clear the problem pane if the tab is closed

3f2e525

feature(diagnostic): add the extension name in the problems

44d9990

puka-tchou marked this pull request as ready for review October 8, 2021 07:29

dev(changelog): add unreleased features

0180fd7

WengerK requested review from ItalyPaleAle and WengerK October 11, 2021 07:05

ItalyPaleAle requested changes Oct 16, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

src/extension.ts Outdated Show resolved Hide resolved

ItalyPaleAle and others added 3 commits October 16, 2021 13:16

Merge branch 'master' into feature/problems

acad1b8

fix: corrects the message and the default severity

ddd961d

Merge branch 'master' into feature/problems

33e5077

puka-tchou requested a review from ItalyPaleAle October 19, 2021 09:06

WengerK force-pushed the feature/problems branch from b07dbc0 to c13fc67 Compare October 26, 2021 08:03

WengerK reviewed Oct 26, 2021

View reviewed changes

test/extension.test.ts Outdated Show resolved Hide resolved

WengerK reviewed Oct 26, 2021

View reviewed changes

test/extension.test.ts Outdated Show resolved Hide resolved

add jest coverage for Diagnostic pane feature

c624907

WengerK force-pushed the feature/problems branch from c13fc67 to c624907 Compare October 26, 2021 10:14

WengerK linked an issue Oct 26, 2021 that may be closed by this pull request

Add bad chars notification to VSCode problems #2

Open

ItalyPaleAle approved these changes Nov 1, 2021

View reviewed changes

Merge branch 'master' into feature/problems

2933a43

docs: update the readme.

8f2a23f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add bad characters to the problems pane. closes #2 #27

feature: add bad characters to the problems pane. closes #2 #27

puka-tchou commented Oct 7, 2021 •

edited

Loading

puka-tchou commented Oct 11, 2021

WengerK commented Oct 11, 2021 •

edited

Loading

puka-tchou commented Oct 12, 2021

ItalyPaleAle left a comment

WengerK commented Oct 19, 2021

puka-tchou commented Oct 20, 2021

WengerK commented Oct 25, 2021

WengerK commented Oct 26, 2021

puka-tchou commented Oct 26, 2021

WengerK commented Oct 26, 2021

ItalyPaleAle commented Oct 26, 2021

ItalyPaleAle commented Nov 1, 2021

WengerK commented Nov 1, 2021

ItalyPaleAle commented Nov 1, 2021

WengerK commented Nov 1, 2021

puka-tchou commented Nov 1, 2021

ItalyPaleAle commented Nov 1, 2021

puka-tchou commented Nov 2, 2021

WengerK commented Nov 2, 2021

WengerK commented Nov 4, 2021

puka-tchou commented Nov 12, 2021

ItalyPaleAle commented Nov 12, 2021

feature: add bad characters to the problems pane. closes #2 #27

Are you sure you want to change the base?

feature: add bad characters to the problems pane. closes #2 #27

Conversation

puka-tchou commented Oct 7, 2021 • edited Loading

puka-tchou commented Oct 11, 2021

WengerK commented Oct 11, 2021 • edited Loading

puka-tchou commented Oct 12, 2021

ItalyPaleAle left a comment

Choose a reason for hiding this comment

WengerK commented Oct 19, 2021

puka-tchou commented Oct 20, 2021

WengerK commented Oct 25, 2021

WengerK commented Oct 26, 2021

puka-tchou commented Oct 26, 2021

WengerK commented Oct 26, 2021

ItalyPaleAle commented Oct 26, 2021

ItalyPaleAle commented Nov 1, 2021

WengerK commented Nov 1, 2021

ItalyPaleAle commented Nov 1, 2021

WengerK commented Nov 1, 2021

puka-tchou commented Nov 1, 2021

ItalyPaleAle commented Nov 1, 2021

puka-tchou commented Nov 2, 2021

WengerK commented Nov 2, 2021

WengerK commented Nov 4, 2021

puka-tchou commented Nov 12, 2021

ItalyPaleAle commented Nov 12, 2021

puka-tchou commented Oct 7, 2021 •

edited

Loading

WengerK commented Oct 11, 2021 •

edited

Loading