feat: add Artwork::color and tests #4
Loading…
Reference in a new issue
No description provided.
Delete branch "liam/colors"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Uses ANSI terminal colors and allows per-character customization to the
foreground and background. Includes a new
Artworkconstructor to colorautomatically.
Artwork::colorand tests 1160f7c719The rest of the code base has been transitioning to
using namespace stdin.cppfiles. Theartwork.cppandartwork_test.cppfiles should do so too. As an added benefit, it will help address some of the long line lengths.@ -82,0 +89,4 @@* first -> foreground color* second -> bacground color*/const std::map<char, const std::pair<const int, const int>> colors = {Since this is a constant, it should be in SCREAMING_SNAKE case like the others.
@ -21,0 +79,4 @@lines = colored_lines;}Artwork::Artwork(const std::string &raw, int offset, const char *raw_fg, const char *raw_bg)This should to be grouped with the other constructor at the top of the file
@ -0,0 +49,4 @@TEST(Artwork, AllBackgrounds) {Artwork art { "01234567\n89ABCDEF\n", 8};art.color(" \n ", "01234567\n89ABCDEF");std::string expected {"\x1b[39;40m0\x1b[39;41m1\x1b[39;42m2\x1b[39;43m3\x1b[39;44m4\x1b[39;45m5\x1b[39;46m6\x1b[39;47m7\x1b[0m\n\x1b[39;100m8\x1b[39;101m9\x1b[39;102mA\x1b[39;103mB\x1b[39;104mC\x1b[39;105mD\x1b[39;106mE\x1b[39;107mF\x1b[0m"};Perhaps look into splitting this over multiple lines? Maybe something like
All of these expected testing strings could benefit from a similar approach (pretty much just wrapping at 80 columns).
1160f7c719tocc3bc2f420cc3bc2f420to69beb930ad69beb930adto0d86b5f7160d86b5f716to7bcf0f1ba1The main changes I'm looking for are just moving most error checking to guard clauses and cleaning up various sections of
color()as a result of that. I'm also a bit confused about some of your test cases which seem to break the expected rules that delimiters must be a length exact (line and total) mask over the uncolored artwork.@ -21,0 +35,4 @@char curr_fg {' '};char curr_bg {' '};string_view fg {raw_fg};string_view bg {raw_bg};See the comment on line 53.
@ -21,0 +38,4 @@string_view bg {raw_bg};ZipRange<string_view, string_view> color_delims {fg, bg};ZipRange<string_view, string_view>::iterator iter_delims {color_delims.begin()};ZipRange<string_view, string_view>::iterator end_delims {color_delims.end()};This is one of those places where
autois really nice and we have plenty of context to know what the real type is. I would also suggest making using of template parameter inference and just useZipRange.@ -21,0 +47,4 @@for (int col {}; col < line_width; ++col, ++iter_delims) {pair<char, char> color_pair {*iter_delims};stringstream color_esc_seq {};color_esc_seq << "\x1b[";This could be easily moved to the if statement where the rest of the escape code is built and might be in a better context for readers.
@ -21,0 +50,4 @@color_esc_seq << "\x1b[";if (color_pair.first == '\n' || color_pair.second == '\n') {throw runtime_error("Unexpected newline");I'd recommend pulling as many error/invalid states to the top of your logic as possible. Here, the uncolored artwork and the delimiters share the same basic processing which is splitting by newlines. I'd recommend making a private helper method that splits lines.
Then I would split the delimiters before processing. This means that no unexpected newlines will appear and checking for the right format can simply be reduced to:
Once the input has passed all of your checks, then you can proceed with your logic. By handling things this way, you should be able to clean up some of the logic code as well.
@ -21,0 +56,4 @@if (!artwork::COLORS.contains(color_pair.first) ||!artwork::COLORS.contains(color_pair.second)) {throw runtime_error("Invalid color delimiter");This too can be checked for at the beginning, however, that would involve a complete iteration over the delimiters so I think it is acceptable to consider it here. Since it can only be triggered when someone is manually editing the source code or future config files, knowing what character caused the error is important (and a time saver for the user). A simple addition of
": (" + color_pair.first + ", " + color_pair.second + ")"might do the trick. Feel free to find a better way to express this of course.@ -21,0 +77,4 @@pair<char, char> color_pair {*iter_delims};if (color_pair.first == '\n' || color_pair.second == '\n') {if (color_pair.first != color_pair.second) {throw runtime_error("Foreground/background not the same length.");See the comment on line 53. In general, it means that this set of nested if statements is not needed which is good because it makes little sense to me. I'll point out that the condition
color_pair.first == '\n' || color_pair.second == '\n'can only fail if the delimiter lines are of a different length than the artwork. This seems like an error state to me and is something I would want to check for before I've already done a bunch of processing (see the aforementioned comment for an example).With the other changes in mind, this whole block seems to be reducible to just
++iter_delims.@ -0,0 +25,4 @@{Artwork art { "foo\nbar\nbaz\n", 3};ASSERT_EQ(str(art), "foo\nbar\nbaz");art.color( " \n \n ", " \n \n ");The provided delimiters don't seem quite right to me. Since the source artwork has 3 newlines the delimiters should too. This appears to be a problem in all of the test cases but not with the actual delimiters in the
artworknamespace.@ -0,0 +79,4 @@ASSERT_EQ(str(art), "foo\nbar\nbaz");ASSERT_THROW({art.color( " \n \n \n ", " \n \n ");},runtime_errorWhy not also check for the correct type of error message?
7bcf0f1ba1to4896e847faView command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.