Skip to content

added highlighted stringify as a public method#120

Merged
jeremy-rifkin merged 2 commits intojeremy-rifkin:mainfrom
riogu:main
Feb 12, 2025
Merged

added highlighted stringify as a public method#120
jeremy-rifkin merged 2 commits intojeremy-rifkin:mainfrom
riogu:main

Conversation

@riogu
Copy link
Copy Markdown
Contributor

@riogu riogu commented Feb 11, 2025

No description provided.

Copy link
Copy Markdown
Owner

@jeremy-rifkin jeremy-rifkin left a comment

Choose a reason for hiding this comment

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

Thanks so much for taking the time to do this! Looks good to me, a couple quick comments below

Comment on lines +88 to +89
std::string highlight(std::string_view expression);
template<typename T>
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Minor readability thing: Can you add a new line between these two methods

src/assert.cpp Outdated

namespace libassert {

[[nodiscard]] std::string highlight(std::string_view expression) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Instead of throwing this at the top of the file, a better place for it would probably be the section around line 360 where color scheme stuff is defined (in general I try to keep the same organization between header and source though I'm not always as diligent about it as I'd like)

src/assert.cpp Outdated
namespace libassert {

[[nodiscard]] std::string highlight(std::string_view expression) {
return libassert::detail::highlight(expression, libassert::color_scheme::ansi_rgb);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Since this is in namespace libassert these don't have to be qualified with libassert::

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i submitted another commit with the changes 👍

Copy link
Copy Markdown
Owner

@jeremy-rifkin jeremy-rifkin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks so much!

@jeremy-rifkin jeremy-rifkin merged commit 9318aef into jeremy-rifkin:main Feb 12, 2025
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.

2 participants