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

Move color parse error message out of hex parser #1937

Merged
merged 8 commits into from
Dec 9, 2024

Conversation

Caellian
Copy link
Collaborator

@Caellian Caellian commented May 25, 2024

Gives a better error message if no parsers (possibly more in future) can parse the color.

This avoids issues like getting a misleading error message when color name parsing didn't work.

I'm making this change until I'm done with logging improvements (a few tweaks left, but not sure when I'll be able to clean up the code).

@github-actions github-actions bot added the sources PR modifies project sources label May 25, 2024
Copy link

netlify bot commented May 25, 2024

Deploy Preview for conkyweb canceled.

Name Link
🔨 Latest commit 171b961
🔍 Latest deploy log https://app.netlify.com/sites/conkyweb/deploys/66524726eb2604000834cbba

@Caellian Caellian changed the title Move color parse message out of hex parser Move color parse error message out of hex parser May 25, 2024
@github-actions github-actions bot added the gh-actions suggest changing GitHub actions label May 25, 2024
@Caellian Caellian force-pushed the tweak/color-parsing-error branch from 5bf3551 to 171b961 Compare May 25, 2024 20:16
@Caellian
Copy link
Collaborator Author

The test seems to be failing because it now catches an error in scan_graph argument parser. I'll fix that too.

Simplifies parsing code, gives a better message and reduces responsibility of
last parser to report errors.

Signed-off-by: Tin Švagelj <tin.svagelj@live.com>
Signed-off-by: Tin Švagelj <tin.svagelj@live.com>
Signed-off-by: Tin Švagelj <tin.svagelj@live.com>
Make ctest print output on failure.

Signed-off-by: Tin Švagelj <tin.svagelj@live.com>
Signed-off-by: Tin Švagelj <tin.svagelj@live.com>
@Caellian Caellian force-pushed the tweak/color-parsing-error branch from 171b961 to fd0ed44 Compare December 9, 2024 00:08
@github-actions github-actions bot removed the gh-actions suggest changing GitHub actions label Dec 9, 2024
Copy link

netlify bot commented Dec 9, 2024

Deploy Preview for conkyweb canceled.

Name Link
🔨 Latest commit fc1d106
🔍 Latest deploy log https://app.netlify.com/sites/conkyweb/deploys/675641fcbe926d00080614dc

@Caellian Caellian force-pushed the tweak/color-parsing-error branch from fd0ed44 to f83c707 Compare December 9, 2024 00:11
@github-actions github-actions bot added the gh-actions suggest changing GitHub actions label Dec 9, 2024
Didn't intend to use CRIT_ERR, however, tests shouldn't crash because
CRIT_ERR calls clean_up from conky.cc.

Signed-off-by: Tin Švagelj <tin.svagelj@live.com>
Signed-off-by: Tin Švagelj <tin.svagelj@live.com>
@github-actions github-actions bot removed the gh-actions suggest changing GitHub actions label Dec 9, 2024
Signed-off-by: Tin Švagelj <tin.svagelj@live.com>
@github-actions github-actions bot added the tests related to project tests label Dec 9, 2024
@@ -196,6 +196,7 @@ TEST_CASE("cpu_percentage and cpu_barval return correct values") {
REQUIRE(cpu_barval(&obj1) == Approx(0.507));

delete[] info.cpu_usage;
info.cpu_usage = nullptr;
Copy link
Collaborator Author

@Caellian Caellian Dec 9, 2024

Choose a reason for hiding this comment

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

This little thing is out of scope, but it causes any tests which attempt using CRIT_ERR to crash. Tests shouldn't use CRIT_ERR because it interrupts their execution, but it's better to handle it explicitly than as a side effect of another test.

@Caellian Caellian merged commit dcbe40b into main Dec 9, 2024
38 checks passed
@Caellian Caellian deleted the tweak/color-parsing-error branch December 9, 2024 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sources PR modifies project sources tests related to project tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants