feat:Make use of coloured output #197
Conversation
src/messages.h
Outdated
| @@ -0,0 +1,14 @@ | |||
| #ifndef MESSAGES_H | |||
There was a problem hiding this comment.
Please do if guards in a consistent manner, like the other headers. For example, SOURCEMETA_JSONSCHEMA_CLI_MESSAGES_H_
| #ifdef _WIN32 | ||
| #include <io.h> | ||
| #define isatty _isatty | ||
| #else |
There was a problem hiding this comment.
Where are we actually checking for TTY in the code?
There was a problem hiding this comment.
We haven't introduced yet . In this pr i have just inlmported the module for tty and color. In next pr we will introduce condition for tty as cli-program detected then we will continue using colors else we will turn off.
There was a problem hiding this comment.
I think you still need the TTY check on this one, otherwise i.e. piping the help text will result in escaped characters
src/main.cc
Outdated
| @@ -85,101 +92,157 @@ Global Options: | |||
|
|
|||
| For more documentation, visit https://github.com/sourcemeta/jsonschema | |||
| )EOF"}; | |||
| // Initialize color usage | |||
| bool use_colors = true; | |||
There was a problem hiding this comment.
We shouldn't have a boolean like this. Instead, each command should check the command line arguments + the TTY check.
There was a problem hiding this comment.
Alright ,in our code i was just going to check ones when the problem starts.Thankyou for guidance i will make sure it checks each time when command run.
src/main.cc
Outdated
|
|
||
| auto jsonschema_main(const std::string &program, const std::string &command, | ||
| const std::span<const std::string> &arguments) -> int { | ||
| if (command == "fmt") { | ||
| const std::span<const std::string> &arguments) -> int |
There was a problem hiding this comment.
All of these changes seem unrelated? They will also break the build, and we check formatting against ClangFormat
There was a problem hiding this comment.
Thankyou ,i was also confused i will now use formated code for our simplicity and build.
There was a problem hiding this comment.
If you run make on your system that should run ClangFormat as per the project's configuration and always get the formatting right
There was a problem hiding this comment.
Yeah i am this one only Thankyou you are great help.😃
src/main.cc
Outdated
| return EXIT_SUCCESS; | ||
| } | ||
| } | ||
|
|
||
| auto main(int argc, char *argv[]) noexcept -> int { | ||
| try { | ||
| auto main(int argc, char *argv[]) noexcept -> int |
There was a problem hiding this comment.
Most of these seem to be unrelated formatting changes?
There was a problem hiding this comment.
I am going to revert al these formatting. Thankyou.
src/messages.h
Outdated
| #include "../include/termcolor/termcolor.hpp" | ||
|
|
||
| // Declare color control as an external variable | ||
| extern bool use_colors; |
There was a problem hiding this comment.
We should avoid these kind of global variables, even more extern ones.
src/messages.cc
Outdated
There was a problem hiding this comment.
Mainly for this PR, I don't think we need these extra helpers. Can you use termcolor on standard streams directly?
There was a problem hiding this comment.
Yes we can do but in further steps ,we again have to change .Is is better to define all colors in one messages page so that it would be easy and maintained. Pls share your thoughts.
There was a problem hiding this comment.
I think termcolor interface is already quite concise and we could use it directly when needed.
include/termcolor/termcolor.hpp
Outdated
There was a problem hiding this comment.
You shouldn't be including untracked external dependencies. Please add them using DEPENDENCIES and https://github.com/sourcemeta/vendorpull
There was a problem hiding this comment.
And once you add it like that, you will need to pull the dependency using CMake's find_package, like we do for other dependencies
There was a problem hiding this comment.
yeaah i got it .Thankyou.
src/main.cc
Outdated
|
|
||
| print_success("Usage: " + std::filesystem::path{program}.filename().string() + " <command> [arguments...]"); | ||
|
|
||
| print_success(std::string(USAGE_DETAILS)); |
There was a problem hiding this comment.
We probably shouldn't colorise the entire thing. Maybe just colorise the usage line above or something like that?
There was a problem hiding this comment.
Lets just color the usage part.
|
Hiii @jviotti whats i am doing wrong ? When i am using make command it is showing 100% test passes but the workflow is breaking...can you guide me. |
Task 1 completed:Create a first PR that introduce the <unistd.h> approach on a helper in utils.h/utils.cc and maybe just uses it to color code "JSON Schema CLI vX.Y.Z" on the help page. Something super simple and without letting users control it yet.
