Idiomatic label printing

[sharpvik]

Closes #2294
I looked at the Colorizer code and included an edge case for labels (which are pretty much all strings that end with a colon like "error:").

Colorizer prints pieces out one by one, so this will not have any unpredictable effects. The piece is only ever considered a label if it is a string that ends with a colon (so "error:" is a label but "this didn't work. usage: bla bla bla" is not a label even though "usage:" has a colon).

I ran all tests and they all pass as well as manually checked the result:

It works.

[sharpvik]

Locally, all tests and checks pass, but in CI it can't deal with imports properly for some reason... Still checking why.

[ldm0]

Thanks! Here are my suggestions.

[ldm0]

Well, this only works if we assume the text pieces doesn't have a trailing : I guess. What about bubbleing the logic up? Editing the start_error() src/parse/errors.rs:400 to may also works.

[ldm0]

BTW, this function is not feature gated so there are some warnings.

[sharpvik]

Thanks! Here are my suggestions.

It's hard to properly change due to the fact that the Colorizer is pretty much guessing what you want it terms of font weight. There is an if clause that sets all red text to be bold.

Do you think it might be better to somehow merge formatting together so that it can be better customized without having to add edge cases to the print method? I could do that.

You know something like passing text + color + width to print or even using a library like 'colored' that allows you to do this:

println!("{}", "this is my error message".red().bold())

[ldm0]

Thanks! Here are my suggestions.

It's hard to properly change due to the fact that the Colorizer is pretty much guessing what you want it terms of font weight. There is an if clause that sets all red text to be bold.

I think the original if clause means all red text needs to be bold rather than guessing(?).

Do you think it might be better to somehow merge formatting together so that it can be better customized without having to add edge cases to the print method? I could do that.

You know something like passing text + color + width to print or even using a library like 'colored' that allows you to do this:

println!("{}", "this is my error message".red().bold())

That would be convenient to use.

Actually my point is that you can change the code like this, which may also work:

diff --git a/src/parse/errors.rs b/src/parse/errors.rs      
index c901f890..4991b801 100644
--- a/src/parse/errors.rs
+++ b/src/parse/errors.rs
@@ -398,8 +398,8 @@ impl Display for Error {
 }

 fn start_error(c: &mut Colorizer, msg: impl Into<String>) {
-    c.error("error:");
-    c.none(" ");
+    c.error("error");
+    c.none(": ");
     c.none(msg);
 }

[sharpvik]

Thanks! Here are my suggestions.

It's hard to properly change due to the fact that the Colorizer is pretty much guessing what you want it terms of font weight. There is an if clause that sets all red text to be bold.

I think the original if clause means all red text needs to be bold rather than guessing(?).

Do you think it might be better to somehow merge formatting together so that it can be better customized without having to add edge cases to the print method? I could do that.

You know something like passing text + color + width to print or even using a library like 'colored' that allows you to do this:

println!("{}", "this is my error message".red().bold())

That would be convenient to use.

Actually my point is that you can change the code like this, which may also work:

diff --git a/src/parse/errors.rs b/src/parse/errors.rs      
index c901f890..4991b801 100644
--- a/src/parse/errors.rs
+++ b/src/parse/errors.rs
@@ -398,8 +398,8 @@ impl Display for Error {
 }

 fn start_error(c: &mut Colorizer, msg: impl Into<String>) {
-    c.error("error:");
-    c.none(" ");
+    c.error("error");
+    c.none(": ");
     c.none(msg);
 }

Oh yeah, that was my initial idea but then I thought it looks weird and also the colon won't be bold in this case but it should be.

[ldm0]

@sharpvik Real perfectionist 😄 . In that case, I think adding extra parameter to indicate bold or not is better?

[sharpvik]

@sharpvik Real perfectionist 😄 . In that case, I think adding extra parameter to indicate bold or not is better?

Yeah, sounds about right :) I'll do that in a bit.

[pksunkara]

This code honestly feels and looks like a hack. I wouldn't merge it as it is right now.

It is because this is the wrong place to fix it. The correct place is here

[sharpvik]

This code honestly feels and looks like a hack. I wouldn't merge it as it is right now.

It is because this is the wrong place to fix it. The correct place is here

I know, I didn't feel good about it either. I'm not deeply familiar with the codebase and I didn't want to bother by asking, so I followed a trail of usages from c.error("error:") to formatter in fmt.rs and that's how I ended up there...