feat:add color and ttl check #199
Conversation
|
@jviotti, you're a great help. But can you assist me with this broken test? |
|
Alright i think i got an error. |
This reverts commit 9de1e58.
|
hiii @jviotti can you assist me for windows workflow. |
| @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.16) | |||
| project(jsonschema VERSION 4.3.0 LANGUAGES CXX) | |||
| list(APPEND CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/cmake") | |||
| include(vendor/noa/cmake/noa.cmake) | |||
|
|
|||
| include_directories(vendor/termcolor/include) | |||
There was a problem hiding this comment.
Note termcolor is a CMake project with its own CMakeLists.txt, so instead of including its files directly here (which is not a best practice), you should create a cmake/FindTermColor.cmake and them here do find_package(TermColor REQUIRED) like we do for other dependencies
There was a problem hiding this comment.
Alright @jviotti got your point.
| #ifdef _WIN32 | ||
| #include <io.h> | ||
| #define isatty _isatty | ||
|
|
There was a problem hiding this comment.
Alright 👍 i will remove it.
|
|
||
| #define fileno _fileno | ||
| #else | ||
|
|
There was a problem hiding this comment.
Alright i will also remove it.
| @@ -88,6 +99,13 @@ For more documentation, visit https://github.com/sourcemeta/jsonschema | |||
|
|
|||
| auto jsonschema_main(const std::string &program, const std::string &command, | |||
| const std::span<const std::string> &arguments) -> int { | |||
| bool use_colors = true; | |||
| if (std::find(arguments.begin(), arguments.end(), "--no-color") != | |||
There was a problem hiding this comment.
We have a parse_options helper function to avoid re-parsing command line arguments in every place. Can you make use of that instead?
Also, you probably want to support a short version of this. Maybe -n?
Finally, you should document this new option on the help page and on docs/
There was a problem hiding this comment.
Alright ,i am getting really exited.
There was a problem hiding this comment.
Actually, just to keep things simpler and merge this sooner, let's avoid --no-color altogether. Just do it purely depending on the TTY stuff. We can then get into parsing that option on specific commands later on
There was a problem hiding this comment.
I am not feeling good,i think my health is not good.I am going home i will work on this in few days. Thanks.
| @@ -117,7 +135,12 @@ auto jsonschema_main(const std::string &program, const std::string &command, | |||
| << sourcemeta::jsonschema::cli::PROJECT_VERSION << "\n"; | |||
| std::cout << "Usage: " << std::filesystem::path{program}.filename().string() | |||
| << " <command> [arguments...]\n"; | |||
| std::cout << USAGE_DETAILS; | |||
| if (use_colors) { | |||
| std::cout << termcolor::yellow << USAGE_DETAILS << termcolor::reset | |||
There was a problem hiding this comment.
I don't think it makes much sense to color out the entire help. Maybe just color out the project name in line 134?
|
@jviotti you tell me why windows build is getting failed? |
There was a problem hiding this comment.
Can you mask all unnecessary files (except LICENSE) using vendorpull? See https://github.com/sourcemeta/vendorpull#masking. Otherwise we are even including screenshots from termcolor here
|
I'm not sure. The error is weird indeed. Let's fix all of the other things, and if it still fails when the PR is ready to merge, I'll take a proper look! |
|
Hii @jviotti, I know this is a bit of an unusual request, but I was wondering if you could share some resources that could help me delve deeper into this topic. |
|
Hey @Pavankumar07s, about what topic specifically? Maybe this page where we list useful resources in general help? https://www.sourcemeta.com/resources/ |
|
I am not feeling good,i think my health is not good.I am going home i will work on this in few days. Thanks. |
|
I have already shared my situation, but I think it was in an unnoticed place. |
checked all the points.
Got 100perset test passed.