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:Termcolor #221

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

Pavankumar07s
Copy link

What kind of change does this PR introduce?

Feature: 1.Added termcolor
2. just colored the version
3.used parse_options helper function to avoid re-parsing command line arguments.
4. added short version -n for --no-color
Issue Number:

Closes #1
Screenshots/videos:
Screenshot from 2025-02-02 15-28-27
Screenshot from 2025-02-02 15-28-40
Screenshot from 2025-02-02 15-28-51

@Pavankumar07s
Copy link
Author

Hello Mr. @jviotti, I am using clang-format to format the code, but the workflow still fails. Could you please guide me?

@jviotti
Copy link
Member

jviotti commented Feb 3, 2025

Hey @Pavankumar07s, does this PR take over #199? Can we close that one if so?

Hello Mr. @jviotti, I am using clang-format to format the code, but the workflow still fails. Could you please guide me?

How are you installing and running clang-format? If you have a too-old version, it might be doing things differently. See https://github.com/sourcemeta/jsonschema/blob/main/.github/workflows/test.yml#L41 for the version we use on CI at any given point in time.

Also note that we run clang-format with a specific configuration file (https://github.com/sourcemeta/core/blob/main/cmake/common/targets/clang-format.config) and if you are using a code editor integration, it might not be picking it up.

I always run it with make (or nmake on Windows). That builds the project and runs the formatter with all the right settings.

${CMAKE_CURRENT_LIST_DIR}/../vendor/termcolor/include)

include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(TermColor
Copy link
Member

Choose a reason for hiding this comment

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

What is this for?

Copy link
Author

Choose a reason for hiding this comment

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

Mr. @jviotti, as you suggested, termcolor has its own CMake file, and I need to create this file to use it. I researched and came up with this approach.

Copy link
Member

Choose a reason for hiding this comment

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

See https://github.com/sourcemeta/jsonschema/pull/221/files#r1941593287. I think you are doing it the hard way

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 of the files that are not essential for the build? See https://github.com/sourcemeta/vendorpull?tab=readme-ov-file#masking

Copy link
Author

Choose a reason for hiding this comment

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

Alright Mr @jviotti

@jviotti
Copy link
Member

jviotti commented Feb 3, 2025

Looking better. Can you also implement a couple of tests cases showing the color escaping codes taking effect, and also the "no color" option doing the right thing?

@Pavankumar07s
Copy link
Author

Alright Mr @jviotti

Copy link
Member

Choose a reason for hiding this comment

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

This is overly complicated and not following the same conventions we have in other Find* files. Plus it's using the _FOUND suffixes that CMake actually expects, relying on targets instead. This works for me and it is pretty much exactly the same as with other dependencies:

if(NOT TermColor_FOUND)
  add_subdirectory("${PROJECT_SOURCE_DIR}/vendor/termcolor")
  set(TermColor_FOUND ON)
endif()

Copy link
Author

Choose a reason for hiding this comment

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

Yes changing to this way.

Copy link
Member

Choose a reason for hiding this comment

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

This seems like an unrelated change?

Copy link
Member

Choose a reason for hiding this comment

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

Let's mask all of these files!

Copy link
Author

Choose a reason for hiding this comment

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

Ohhk sir , reading the docs.

@jviotti
Copy link
Member

jviotti commented Feb 4, 2025

Maybe to simplify things, you can first send a PR integrating TermColor and linking to it? We can get that merged, and then focus on the actual option?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make use of coloured output
2 participants