-
-
Notifications
You must be signed in to change notification settings - Fork 909
Improve Makefile readability by symbolizing ANSI #1425
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
base: main
Are you sure you want to change the base?
Conversation
deckstose
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see much value in this.
| @@ -0,0 +1 @@ | |||
| \n \033[38;5;196m██████\033[38;5;240m╗ \033[38;5;196m████████\033[38;5;240m╗ \033[38;5;196m██████\033[38;5;240m╗ \033[38;5;196m██████\033[38;5;240m╗\n \033[38;5;160m██\033[38;5;239m╔══\033[38;5;160m██\033[38;5;239m╗╚══\033[38;5;160m██\033[38;5;239m╔══╝\033[38;5;160m██\033[38;5;239m╔═══\033[38;5;160m██\033[38;5;239m╗\033[38;5;160m██\033[38;5;239m╔══\033[38;5;160m██\033[38;5;239m╗ \033[38;5;160m██\033[38;5;239m╗ \033[38;5;160m██\033[38;5;239m╗\n \033[38;5;124m██████\033[38;5;238m╔╝ \033[38;5;124m██\033[38;5;238m║ \033[38;5;124m██\033[38;5;238m║ \033[38;5;124m██\033[38;5;238m║\033[38;5;124m██████\033[38;5;238m╔╝ \033[38;5;124m██████\033[38;5;238m╗\033[38;5;124m██████\033[38;5;238m╗\n \033[38;5;88m██\033[38;5;237m╔══\033[38;5;88m██\033[38;5;237m╗ \033[38;5;88m██\033[38;5;237m║ \033[38;5;88m██\033[38;5;237m║ \033[38;5;88m██\033[38;5;237m║\033[38;5;88m██\033[38;5;237m╔═══╝ ╚═\033[38;5;88m██\033[38;5;237m╔═╝╚═\033[38;5;88m██\033[38;5;237m╔═╝\n \033[38;5;52m██████\033[38;5;236m╔╝ \033[38;5;52m██\033[38;5;236m║ ╚\033[38;5;52m██████\033[38;5;236m╔╝\033[38;5;52m██\033[38;5;236m║ ╚═╝ ╚═╝\n \033[38;5;235m╚═════╝ ╚═╝ ╚═════╝ ╚═╝ \033[1;3;38;5;240mMakefile v1.6\033[0m | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just pollutes the repository
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could have said that in a nicer way:
If we split the banner out in a file, this should go in a dedicated folder, conventionally this is assets (the pictures in the README from Img should go in that folder, too).
Saying that I don't see why adding a dedicated file for a one line variable is necessary (it's not used anywhere else, that's the only reason I would deem this useful).
The escape codes into variables looks nice tho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we split the banner out in a file, this should go in a dedicated folder, conventionally this is
assets(the pictures in the README fromImgshould go in that folder, too).
Agreed! I originally put it in assets/ but then "simplified" for this PR.
Your idea of moving all assets (including Img/) makes perfect sense.
The bigger solution is to have that ANSI banner sequence appear once as a single source of truth. But it's already embedded in the cpp... so I didn't want to override that.
aristocratos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good effort to simplify, but needs to be tested on all platforms before merging.
| @@ -0,0 +1 @@ | |||
| \n \033[38;5;196m██████\033[38;5;240m╗ \033[38;5;196m████████\033[38;5;240m╗ \033[38;5;196m██████\033[38;5;240m╗ \033[38;5;196m██████\033[38;5;240m╗\n \033[38;5;160m██\033[38;5;239m╔══\033[38;5;160m██\033[38;5;239m╗╚══\033[38;5;160m██\033[38;5;239m╔══╝\033[38;5;160m██\033[38;5;239m╔═══\033[38;5;160m██\033[38;5;239m╗\033[38;5;160m██\033[38;5;239m╔══\033[38;5;160m██\033[38;5;239m╗ \033[38;5;160m██\033[38;5;239m╗ \033[38;5;160m██\033[38;5;239m╗\n \033[38;5;124m██████\033[38;5;238m╔╝ \033[38;5;124m██\033[38;5;238m║ \033[38;5;124m██\033[38;5;238m║ \033[38;5;124m██\033[38;5;238m║\033[38;5;124m██████\033[38;5;238m╔╝ \033[38;5;124m██████\033[38;5;238m╗\033[38;5;124m██████\033[38;5;238m╗\n \033[38;5;88m██\033[38;5;237m╔══\033[38;5;88m██\033[38;5;237m╗ \033[38;5;88m██\033[38;5;237m║ \033[38;5;88m██\033[38;5;237m║ \033[38;5;88m██\033[38;5;237m║\033[38;5;88m██\033[38;5;237m╔═══╝ ╚═\033[38;5;88m██\033[38;5;237m╔═╝╚═\033[38;5;88m██\033[38;5;237m╔═╝\n \033[38;5;52m██████\033[38;5;236m╔╝ \033[38;5;52m██\033[38;5;236m║ ╚\033[38;5;52m██████\033[38;5;236m╔╝\033[38;5;52m██\033[38;5;236m║ ╚═╝ ╚═╝\n \033[38;5;235m╚═════╝ ╚═╝ ╚═════╝ ╚═╝ \033[1;3;38;5;240mMakefile v1.6\033[0m | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last part "Makefile v1.6" should probably be added to the BANNER_FILE variable instead so it's not missed if updating the Makefile.
All messy inline
\033[jazz is removed.In refactoring the Makefile, I did stumble across a couple bugs. However, I tried not to change any logic in this branch except for some inconsistent formatting. For example, when outputting
Message...the trailing ... were colorized inconsistently. So I switched all trailing ... to non-formatted for consistency. Could also leave it bold, if maintainers prefer that?Also, the
$(P)symbol is no longer needed.And at least one unreferenced
$$TSTAMPwas removed.And the new
file_with_size()hides du stderr. Is that right?And the new
step_duration()replaces a reference todatewith$(DATE_CMD). But I didn't do this globally. There are a few more, such as in@TSTAMP=.I've spot tested on linux only.