extend unknown color error message #147
Conversation
There was a problem hiding this comment.
- I highly recommend accepting this PR!
- See my suggested change if it might be useful to inform the user about any unprintable or white-space characters in the input.
- I have included a test case YAML in [ci] Include a set of input files for regression testing #63 (comment) that also demonstrates how useful this PR is for showing diagnostic output about the current bug [bug] TEL and TELALT contains some unknown color codes #144.
src/wireviz/wv_colors.py
Outdated
| @@ -112,7 +112,7 @@ def get_color_hex(input, pad=False): | |||
| try: | |||
| output = [_color_hex[input[i:i + 2]] for i in range(0, len(input), 2)] | |||
| except KeyError: | |||
| print("Unknown color specified") | |||
| print(f"Unknown color specified: {input}") | |||
There was a problem hiding this comment.
Consider also something like print(f'Unknown color specified: {bytes(input, "utf-8")}') to catch any unprintable characters. Edit: (or white-space characters that need quotes to be seen)
There was a problem hiding this comment.
I tried that, but I'm not sure if I like the byte indicator around the color string. Unknown color specified: b'OGYW' But I will include it if you want 😄
There was a problem hiding this comment.
- I agree, and it's easy to strip out the leading byte indicator by using
str(bytes(input, "utf-8"))[1:]instead if that is the only thing you don't like. - At the cost of an extra code line and an extra
if-else, it is also possible (if you want, but not required IMHO - a matter of taste, I guess) to avoid the quotes when they are not strictly needed:
quoted = str(bytes(input, "utf-8"))[1:] # Skip byte indicator, but keep quotes
print(f'Unknown color specified: {input if input.strip() == quoted[1:-1] else quoted}')However, I don't insist. It's just a suggestion in case someone find it useful. 😄
There was a problem hiding this comment.
Maybe I'm out of the loop, but why don't we stick to the suggested
print(f'Unknown color specified: {input}')?
Are we really worried people will try to use emojis and non-printable characters as colors? 😛
For stray whitespace characters,
print(f'Unknown color specified: "{input}"')should be enough to catch them.
There was a problem hiding this comment.
Any of them are OK with me. I just wanted to menton ideas that have been useful in other projects, but you decide what is needed in this project. My reply was not intended as a fight for my suggestion, but simply a correction of the draw-back that was commented.
There was a problem hiding this comment.
OK, just trying to understand the motivation for the idea. I think for now it's safe to keep it as-is :)
There was a problem hiding this comment.
- Please try to limit the subject line to 50 characters, or maximum 72 (the rest of that page is also very useful reading).
- The bug below is just a simple oversight.
- I have already answered your comment to my previous review by updating my messages in that thread - no need to repeat them here - see links below.
| try: | ||
| output = [_color_hex[input[i:i + 2]] for i in range(0, len(input), 2)] | ||
| output = [_color_hex[padded[i:i + 2]] for i in range(0, len(input), 2)] |
There was a problem hiding this comment.
- Test: YAML input from [ci] Include a set of input files for regression testing #63 (comment)
- Bug: The returned output list length is now
len(input) / 2, i.e. ignoring any extra entries inpadded - Fix: Replace
len(input)withlen(padded)
| except KeyError: | ||
| print(f"Unknown color specified: {input}") | ||
| print(f'Unknown color specified: {input}') |
There was a problem hiding this comment.
See my updated comments about white-space in #147 (comment), and about byte indicator and quotes in #147 (comment)
print out the color string that cannot be found so the user has it easier to fix typos.