42School / norminette

Official 42 norminette
MIT License
926 stars 138 forks source link

Change error displaying and add colors! #490

Closed zylosophe closed 2 months ago

zylosophe commented 6 months ago

294 is already providing something but it seems that it's not compatible with the changes made to the output code after the last released version (3.3.55), so I made a version which can directly be merged into master.

Obvious from the title: these changes make the norm error hunting colorful (also easier but that's just a side effect)

Summary

The OK! and Error! are put at the left of the file names as [OK] and [KO]. This way they are aligned and easier to read.

The [OK] and [KO] are colored in red and green. The Error: lines are red. The Notice: lines are yellow. This way, seeing a lot of errors is even scarier than before.

About the tests

I have no idea how they work, so um if there is something to change, could someone help me on that or send me docs?

NiumXp commented 6 months ago

Hello, @zylosophe!

I'm working on two similars changes, the --no-color flag (necessary to fix all tests for your changes) with an API Color to use it (will be like Color.red(text) and if --no-color is True, it will return just text) and a better HumanizedErrorsFormatter (the actually formatter will be renamed ShortErrorsFormatter): image Note that I removed the status

Preview of your changes

image I'm not sure if coloring all the line is good, maybe just the prefix (Error and Notice) and the message

zylosophe commented 6 months ago

Hello!

I like the idea of seeing the line with the error, will it be a way to use the old format, like a --short flag?

Did you already wrote/included Color? I can rewrite my code on a commit where Color already exists.

I noticed afterwhise than coloring all the lines was maybe too much ^^ I rewrote another code using the same format than #294 (only difference is that the [OK] can be yellow if there is a Notice in the file)

NiumXp commented 6 months ago

I like the idea of seeing the line with the error, will it be a way to use the old format, like a --short flag?

Yes, it will! We are able to change the output using --format=(short|humanized|json). The --short can be an alias for --format=short too.

Did you already wrote/included Color? I can rewrite my code on a commit where Color already exists.

I did not, but you can see a git diff of what I'm talking about:

diff --git a/norminette/__main__.py b/norminette/__main__.py
index 60143a6..382d8d6 100644
--- a/norminette/__main__.py
+++ b/norminette/__main__.py
@@ -11,7 +11,8 @@ from norminette.lexer import Lexer, TokenError
 from norminette.exceptions import CParsingError
 from norminette.registry import Registry
 from norminette.context import Context
-from norminette.tools.colors import colors
+from norminette.color import Color
+from norminette import color

 import subprocess

@@ -74,10 +75,13 @@ def main():
         help="formatting style for errors",
:...skipping...
diff --git a/norminette/__main__.py b/norminette/__main__.py
index 60143a6..382d8d6 100644
--- a/norminette/__main__.py
+++ b/norminette/__main__.py
@@ -11,7 +11,8 @@ from norminette.lexer import Lexer, TokenError
 from norminette.exceptions import CParsingError
 from norminette.registry import Registry
 from norminette.context import Context
-from norminette.tools.colors import colors
+from norminette.color import Color
+from norminette import color

 import subprocess

@@ -74,10 +75,13 @@ def main():
         help="formatting style for errors",
         default="humanized",
     )
+    parser.add_argument("--no-color", action="store_false", help="Disable all output colors", default=True)
     parser.add_argument("-R", nargs=1, help="compatibility for norminette 2")
     args = parser.parse_args()
     registry = Registry()

+    color._enabled = args.no_color  # type: ignore
+
     format = next(filter(lambda it: it.name == args.format, formatters))
     files = []
     debug = args.debug
@@ -131,7 +135,7 @@ def main():
             context = Context(file, tokens, debug, args.R)
             registry.run(context)
         except (TokenError, CParsingError) as e:
-            print(file.path + f": Error!\n\t{colors(e.msg, 'red')}")
+            print(file.path + f": Error!\n\t{Color.red(e.msg)}")
             sys.exit(1)
         except KeyboardInterrupt:
             sys.exit(1)
diff --git a/norminette/color.py b/norminette/color.py
new file mode 100644
index 0000000..d6c10d4
--- /dev/null
+++ b/norminette/color.py
@@ -0,0 +1,20 @@
+_enabled = True
+
+
+def _color_fn_factory(n: int, /):
+    def fn(text: str) -> str:
+        return f"\x1b[0;{n}m{text}\x1b[0m" if _enabled else text
+    return staticmethod(fn)
+
+
+class Color:
+    _ = _color_fn_factory
+
+    gray = _(30)
+    red = _(31)
+    green = _(32)
+    yellow = _(33)
+    blue = _(34)
+    magenta = _(35)
+    cyan = _(36)
+    white = _(37)
diff --git a/norminette/context.py b/norminette/context.py
index 8d216a2..ccf5bad 100644
--- a/norminette/context.py
+++ b/norminette/context.py
@@ -3,7 +3,7 @@ from dataclasses import dataclass, field
 from norminette.exceptions import CParsingError
 from norminette.norm_error import NormError, NormWarning
 from norminette.scope import GlobalScope, ControlStructure
-from norminette.tools.colors import colors
+from norminette.color import Color

 types = [
     "CHAR",
@@ -286,7 +286,7 @@ class Context:
         if self.debug < 2:
             return
         print(
-            f"{colors(self.file.basename, 'cyan')} - {colors(rule, 'green')} \
+            f"{Color.cyan(self.file.basename)} - {Color.green(rule)} \
 In \"{self.scope.name}\" from \
 \"{self.scope.parent.name if self.scope.parent is not  None else None}\" line {self.tokens[0].pos[0]}\":"
         )
diff --git a/norminette/tools/colors.py b/norminette/tools/colors.py
deleted file mode 100644
index e6a0da9..0000000
--- a/norminette/tools/colors.py
+++ /dev/null
@@ -1,40 +0,0 @@
-def colors(text, *argv):
-    options = {
-        "bold": 1,
-        "dim": 2,
-        "underline": 4,
-        "blink": 5,
-        "reverse": 7,
-        "hidden": 8,
-        "reset_bold": 21,
-        "reset_dim": 22,
-        "reset_underlined": 24,
-        "reset_blink": 25,
-        "reset_reverse": 27,
-        "reset_hidden": 28,
-        "default_fg": 39,
-        "black": 30,
-        "red": 31,
-        "green": 32,
-        "yellow": 33,
-        "blue": 34,
-        "magenta": 35,
-        "cyan": 36,
-        "light_gray": 37,
-        "dark_gray": 90,
-        "light_red": 91,
-        "light_green": 92,
-        "light_yellow": 93,
-        "light_blue": 94,
-        "light_magenta": 95,
-        "light_cyan": 96,
-        "white": 97,
-        "reset_all": 0,
-    }
-    reset = "\u001b[0m"
-    tmp = []
-    for arg in argv:
-        tmp.append(str(options.get(arg, 0)))
-    sep = ";"
-    res = f"\u001b[{sep.join(tmp)}m{text}{reset}"
-    return res
zylosophe commented 6 months ago

I'm using \e[91m instead of \e[31m, it's clearer, at least on my terminal. Don't know which one is better

Asandolo commented 2 months ago

Hello ! we chose to merge this MR for color, thanks you !