Fix astyle Display Language Dependency & Makefile Colored Output #9441
Conversation
The shell script which checks the style relies on greping for the keyword "Formatted" in the output of astyle. But the program has localization support and will output in other languages e.g. german. This leads to all style checks always succeeding. I only tested this on Windows in Cygwin but I can imagine the problem also exists in non-english Ubuntu installations. Solution is the parameter --formatted of astyle which only produces any output if there was something to fix. This allows for a display language independent condition for an empty string inside the shell script.
Force interpretation of backslash escapes with the parameter -e of echo. Switch to a lighter blue because on certain terminals default blue is hard to read on black background.
MaEtUgR
changed the title
MaEtUgR
force-pushed
the
pr-fix-astyle-language-dependency
branch
from
bc3f4d6 to
ef3d170
Compare
MaEtUgR
changed the title
|
I just manually tested on linux and the escape parameter gets put out to the terminal |
It seems that on linux only inside a makefile the parameter after the echo command gets printed if no single quoted sting comes afterwards so I had to switch to single quotes such that I can use the parameter.
MaEtUgR
changed the title
|
Fixed:
I successfully tested also on linux now. |
Tools/astyle/check_code_style.sh
Outdated
| ${DIR}/fix_code_style.sh --dry-run $FILE | grep --quiet Formatted | ||
| if [[ $? -eq 0 ]]; then | ||
| CHECK_FAILED=$(${DIR}/fix_code_style.sh --dry-run --formatted $FILE) | ||
| if [[ $CHECK_FAILED ]]; then |
There was a problem hiding this comment.
Can you check for an empty string like this: if [ -n "$CHECK_FAILED" ]; then?
There was a problem hiding this comment.
Makes sense, I changed it.
For anyone else interested here's the explanation for the -n parameter in the man pages of test: https://linux.die.net/man/1/test
Thanks to @bkueang 's review comment I switched to an explicit check for an empty sting instead of a condition that could be theoretically true in other cases and is less readable. Type "man test" on your terminal to read up what -n stands for.
|
@MaEtUgR M
This change introduce a new issue in MacOS console: |
|
@haozhang-yuneec Thanks for reporting! Unfortunately I have no access to a mac to test this. I assumed it would behave the same as a linux terminal... (Why does every console have to be different?) |
This PR solves two problems which I found during Cygwin Toolchain usage:
astyle check passes in any case
The shell script which checks the style relies on greping for the keyword "Formatted" in the output of astyle. But the program has localization support and will output in other languages e.g. german. This leads to all style checks always succeeding. I only tested this on Windows in Cygwin but I can imagine the problem also exists on non-english Ubuntu installations.
Solution: is the parameter --formatted of astyle which only produces any output if there was something to fix. This allows for a display language independent condition for an empty string inside the shell script.
Makefile colors do not work e.g.
\033[0;34mFormatting with astyle \033[mCertain terminal combinations like cygwin - mintty - xterm - bash do not escape colors by default.
Solution: Force interpretation of backslash escapes with the parameter -e of echo.
I also switched the colored messages to a lighter blue (94 instead of 34) because in certain terminals default blue is hard to read on black background while on linux both blues were easy to read (see pictures).