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

feat:add color and ttl check #199

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Alright @jviotti got your point.

# Options
option(JSONSCHEMA_TESTS "Build the JSON Schema CLI tests" OFF)
option(JSONSCHEMA_TESTS_CI "Build the JSON Schema CLI CI tests" OFF)
Expand Down
1 change: 1 addition & 0 deletions DEPENDENCIES
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ hydra https://github.com/sourcemeta/hydra a4a74f3cabd32f2f829f449d67339dac33f991
alterschema https://github.com/sourcemeta/alterschema 92e370ce9c1f0582014b54d43e388ee012dfe13d
jsonbinpack https://github.com/sourcemeta/jsonbinpack d777179441d3c703e1fda1187742541aa26836b5
blaze https://github.com/sourcemeta/blaze 4db8309470369332d3d0658ade9402a37abe418e
termcolor https://github.com/ikalnytskyi/termcolor.git v2.1.0
25 changes: 24 additions & 1 deletion src/main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,17 @@
#include "command.h"
#include "configure.h"

#ifdef _WIN32
#include <io.h>
#define isatty _isatty

Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary blank line?

Copy link
Author

Choose a reason for hiding this comment

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

Alright 👍 i will remove it.

#define fileno _fileno
#else

Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary blank line?

Copy link
Author

Choose a reason for hiding this comment

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

Alright i will also remove it.

#include <unistd.h>
#endif
#include <termcolor/termcolor.hpp>

constexpr std::string_view USAGE_DETAILS{R"EOF(
Global Options:

Expand Down Expand Up @@ -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") !=
Copy link
Member

Choose a reason for hiding this comment

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

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/

Copy link
Author

Choose a reason for hiding this comment

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

Alright ,i am getting really exited.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

arguments.end()) {
use_colors = false;
} else if (!isatty(fileno(stdout))) {
use_colors = false;
}
if (command == "fmt") {
return sourcemeta::jsonschema::cli::fmt(arguments);
} else if (command == "frame") {
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it makes much sense to color out the entire help. Maybe just color out the project name in line 134?

<< "\n";
} else {
std::cout << USAGE_DETAILS << "\n";
}
return EXIT_SUCCESS;
}
}
Expand Down
1 change: 1 addition & 0 deletions vendor/termcolor/.mailmap
Copy link
Member

Choose a reason for hiding this comment

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

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

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

58 changes: 58 additions & 0 deletions vendor/termcolor/CMakeLists.txt

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 31 additions & 0 deletions vendor/termcolor/LICENSE

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

236 changes: 236 additions & 0 deletions vendor/termcolor/README.rst

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions vendor/termcolor/cmake/config.cmake.in

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Binary file added vendor/termcolor/docs/_static/example.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Loading