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

Change error displaying and add colors! #490

Closed
wants to merge 3 commits into from

Conversation

zylosophe
Copy link

#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
Copy link
Contributor

NiumXp commented Mar 1, 2024

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

- For each file, the "OK!" and "Error!" are changed
  to [OK] and [KO] and moved to the left of the
  file names.
- The [OK] are green and the [KO] are red.
- If there is a "Notice:" for a file,
  the [OK] becomes yellow.
@zylosophe
Copy link
Author

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
Copy link
Contributor

NiumXp commented Mar 2, 2024

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
  • Removed norminette/tools/colors.py
  • Created Color class in norminette/color.py
  • Added --no-color flag
  • Fixed old codes that is using colors(...) (updated to Color.<color>(...)

norminette/errors.py Outdated Show resolved Hide resolved
@zylosophe
Copy link
Author

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 Asandolo closed this Jun 20, 2024
@Asandolo
Copy link

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

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

Successfully merging this pull request may close these issues.

3 participants