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

Add color echo functions to stdlib #308

Merged
merged 17 commits into from
Jul 23, 2024

Conversation

CymDeveloppement
Copy link
Member

here are some new functions for color echo.

echo functions:

echo_esc an echo -e wrapper
by default echo do not interpret escaped sequence, and is a good choice.
I think Amber should have the same logic, no escaped sequence by default.

color_echo print a message with a style code

info display a blue background text
success display a green background text
warning display a yellow background text
error display a red background text and exit if exit_code is greater than 0

format text functions:
shell_text return a text sequence with foreground, background and style
bold_text return a bold text sequence
italic_text return a italic text sequence
underlined_text return a underlined text sequence

I need your opinion, maybe this is too many function

related to #306 and #290

@Mte90
Copy link
Member

Mte90 commented Jul 15, 2024

We were discussing this morning about this proposal #306

@CymDeveloppement
Copy link
Member Author

We were discussing this morning about this proposal #306

yes I saw
I had already worked on it, I was waiting #291.
I will update this pr when #291 was merged

Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small changes

src/std/main.ab Outdated Show resolved Hide resolved
src/std/main.ab Outdated Show resolved Hide resolved
@b1ek
Copy link
Member

b1ek commented Jul 16, 2024

that would have to be in its own file tho, since #291 was merged

@Mte90
Copy link
Member

Mte90 commented Jul 17, 2024

Just before approve this PR, we have #306 that is proposing a new syntax that will be used for stuff like this PR.

We want to have those methods until that syntax get supported?

@CymDeveloppement
Copy link
Member Author

Just before approve this PR, we have #306 that is proposing a new syntax that will be used for stuff like this PR.

We want to have those methods until that syntax get supported?

I think is not a good idea to use echo -e by default, this can give unexpected results.
for example : echo -e "\version" output :


ersion

this means that the user must understand this mechanism to use the echo command
And there are specific escape sequence for xterm.

this means that user inputs would have to be escaped

i think is more easy if echo command do not interpret escape sequence

@b1ek
Copy link
Member

b1ek commented Jul 17, 2024

echo -e is also sometimes echo -n on some platforms

@CymDeveloppement
Copy link
Member Author

CymDeveloppement commented Jul 17, 2024

echo -e is also sometimes echo -n on some platforms
@b1ek
I didn't know that, do you have an example ?

@Ph0enixKM
Copy link
Member

Ph0enixKM commented Jul 18, 2024

I'm still thinking if what I proposed first (to add print prefix). Now in the hindsight I think that echo would be a better fit.

Example: echo_info

Sorry for that back and fourth.

@Ph0enixKM
Copy link
Member

Since printf is also a part of POSIX, we could use that instead of echo -e:

printf "\033[31mERROR: This is an error\033[0m\n"

We need to remember to add the newline character in the end though. What do you think?

@CymDeveloppement
Copy link
Member Author

Since printf is also a part of POSIX, we could use that instead of echo -e:

printf "\033[31mERROR: This is an error\033[0m\n"

We need to remember to add the newline character in the end though. What do you think?

printf can easily failed and the printf C format can return unexpected result.
The user should also understand C format and printf escaping

this :

printf "\033[31mERROR: This is %T an error\033[0m\n"

fail :

bash: printf: « T » : caractère de format non permis

and this :

printf "\033[31mERROR: This is %an  error\033[0m\n"

return this :

ERROR: This is 0x0p+0n  error

i think echo is better than printf

@CymDeveloppement
Copy link
Member Author

I'm still thinking if what I proposed first (to add print prefix). Now in the hindsight I think that echo would be a better fit.

Example: echo_info

Sorry for that back and fourth.

👍
i think also echo_ is a better prefix.
especially if we keep echo

@Ph0enixKM
Copy link
Member

Ph0enixKM commented Jul 20, 2024

I agree with @CymDeveloppement. I just checked that echo -e isn't part of POSIX (for instance it's not supported on debian's dash). Each shell has different implementation of it. On Bash it works as expected but if we target SH then we have no idea if this command will even work.

The solution would be to use printf and replace all instances of % in the string and replace them with %%. This way we are using the POSIX compatible command:

text="This is %T a test"
printf "${text//%/%%}\n"
# This is %T a test

Here is a little test that I've made:
image

Copy link
Member

@Mte90 Mte90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is perfect for now as we are in alpha, in the future a way to change the colors will be needed for the various functions.

dev.sh Outdated Show resolved Hide resolved
@CymDeveloppement
Copy link
Member Author

now all function use printf, i have also integrate new necessary function.
printf(format: Text, args: [Text]) this function is a wrapper of printf bash function
printf_escape(text: Text) correctly escape for use with printf directly in format argument
for example : printf(text_bold("%t \version")) output this bold text : %t \version and do not interpret special character

@CymDeveloppement
Copy link
Member Author

printf function can close #271

src/std/env.ab Outdated Show resolved Hide resolved
Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small changes

src/std/env.ab Outdated Show resolved Hide resolved
src/std/env.ab Outdated Show resolved Hide resolved
src/std/env.ab Outdated
Comment on lines 92 to 123
pub fun text_bold(message: Text): Text {
return "\e[1m{printf_escape(message)}\e[0m"
}

pub fun text_italic(message: Text): Text {
return "\e[3m{printf_escape(message)}\e[0m"
}

pub fun text_underlined(message: Text): Text {
return "\e[4m{printf_escape(message)}\e[0m"
}

pub fun color_echo(message: Text, color: Num): Null {
printf("\e[{color as Text}m%s\e[0m\n", [message])
}

pub fun echo_info(message: Text): Null {
printf("\e[1;3;97;44m %s \e[0m\n", [message])
}

pub fun echo_success(message: Text): Null {
printf("\e[1;3;97;42m %s \e[0m\n", [message])
}

pub fun echo_warning(message: Text): Null {
printf("\e[1;3;97;43m %s \e[0m\n", [message])
}

pub fun error(message: Text, exit_code: Num = 1): Null {
printf("\e[1;3;97;41m %s \e[0m\n", [message])
if exit_code > 0 : exit(exit_code)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change the \e to \033? Just so that everything is uniform

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\033 not work, i think is a rust problem.
Error: Error { kind: InvalidInput, message: "nul byte found in provided data" }
I used the hexadecimal notation instead

Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Good job

@Ph0enixKM Ph0enixKM merged commit 969d06c into amber-lang:master Jul 23, 2024
1 check passed
@CymDeveloppement CymDeveloppement deleted the color_echo branch July 23, 2024 12:07
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.

[Feature] Text coloring in "echo" function [Bug] No way to print a string without newlines
4 participants