feat(script): add formatting for script failure #2596

indev29 · 2022-02-18T18:13:17Z

What type of PR is this? (check all applicable)

Description

The behavior is not changed unless the feature is used. Old configuration files should work the same way.

This PR adds formatting for script failure (when exited with non-zero exit status):

format-fail = <label-fail>
; and all the others format-fail-*

label-fail = %output%
; and all the others label-fail-*

Falls back to format, if format-fail isn't specified.

The feature is disabled for tail=true scripts, since they either run indefinitely (so exit status doesn't have a change to be used) or stop for an interval that should be relatively short compared to the lifetime of a script. In both scenarios, exit status is received too late for it to be useful so there's no way to implement meaningful formatting for those scripts.

Related Issues & Documents

Closes #2588

Documentation (check all applicable)

This PR requires changes to the Wiki documentation (describe the changes)
This PR requires changes to the documentation inside the git repo (please add them to the PR).
Does not require documentation changes

The following should be added to Modules/script/Additional Formatting:

; Format for script failure (when exited with non-zero exit status)
; If not specified, 'format' is used instead
; Available tags:
;   <label-fail> (default)
format-fail = <label-fail>
format-fail-background = #900
format-fail-foreground = #fff
format-fail-padding = 4

; Available tokens:
;   %output%
; Default: %output%
label-fail = %output:0:15:...%

codecov · 2022-02-20T15:14:40Z

Codecov Report

Merging #2596 (6c97a30) into master (74067c5) will increase coverage by 0.14%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2596      +/-   ##
==========================================
+ Coverage   11.78%   11.93%   +0.14%     
==========================================
  Files         153      153              
  Lines       11265    11297      +32     
==========================================
+ Hits         1328     1348      +20     
- Misses       9937     9949      +12

Flag	Coverage Δ
unittests	`11.93% <0.00%> (+0.14%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/adapters/script_runner.cpp	`0.00% <0.00%> (ø)`
src/modules/script.cpp	`0.00% <0.00%> (ø)`
src/tags/action_context.cpp	`100.00% <0.00%> (ø)`
include/tags/action_context.hpp	`100.00% <0.00%> (ø)`
src/tags/dispatch.cpp	`90.52% <0.00%> (+0.99%)`	⬆️
include/components/types.hpp	`6.66% <0.00%> (+4.44%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74067c5...6c97a30. Read the comment docs.

patrick96

Works very nicely, thanks a lot! 😃

I mainly have some "cosmetic" nitpicks, otherwise everything looks great.

Please also add an entry in the changelog in the Added section: https://github.com/polybar/polybar/blob/master/CONTRIBUTING.md#changelog

src/adapters/script_runner.cpp

include/modules/script.hpp

patrick96 · 2022-02-20T15:25:33Z

include/adapters/script_runner.hpp

@@ -12,8 +12,8 @@ POLYBAR_NS
 class script_runner {
 public:
  using interval = std::chrono::duration<double>;
-  script_runner(std::function<void(void)> on_update, const string& exec, const string& exec_if, bool tail,
-      interval interval, const vector<pair<string, string>>& env);
+  script_runner(std::function<void(void)> on_update, string exec, string exec_if, bool tail, interval interval,


I don't agree with using rvalues here. You don't gain anything since you still have a copy (now it happens when calling the constructor instead of during the constructor).

Even when using rvalue references, from a performance standpoint, the script_runner is only executed when the module starts up and the two strings are generally no more than 100 bytes.

Unless you there is a good reason for this, I'd rather keep using const references.

Well, i think it's kind of a personal preference. Passing an argument by-value offers clearer function interface by explicitly stating that argument will be copied when passed to the function. Const reference, on the other hand, only suggests, that it will and doesn't really specify the nature of interaction with the argument.

Performance-wise, when copy of the argument is desired inside a function, passing by value and then moving is better than passing by const reference, since if rvalue is passed, binding to const reference will always copy instead of just moving. Although, as you noted, it doesn't really matter in this case.

void f1(std::string s); void f2(const std::string s); auto s = std::string(); f1(std::move(s)); // move f2(std::move(s)); // copy

The reason I made this change is because I tend to generally follow this principle, mostly because of clearer interface, than performance reasons. Didn't think much there, just an automatic reaction. I'll roll this one back to const references.

Thanks for the explanation :)

I guess it really is a personal preferences. I would rather not expose the fact that a copy is made in the interface because if the copy becomes unnecessary in the future, it changes the interface and you may have to update all users.

src/modules/script.cpp

patrick96

Alright, just one minor change, then we can merge this :)

src/adapters/script_runner.cpp

patrick96

Perfect!

patrick96 · 2022-02-23T14:01:42Z

Thanks a lot!

Made it just in time for the 3.6 release ;)

patrick96 requested changes Feb 20, 2022

View reviewed changes

indev29 requested a review from patrick96 February 22, 2022 08:07

indev29 added 3 commits February 22, 2022 15:09

feat(script): add formatting for script failure

5a6cba2

refactor: address review comments

a11242d

doc: add changelog entry

45ed0c1

indev29 force-pushed the feat-2588-script-module-formatting branch from 6852db9 to 45ed0c1 Compare February 22, 2022 08:10

patrick96 requested changes Feb 22, 2022

View reviewed changes

src/adapters/script_runner.cpp Outdated Show resolved Hide resolved

refactor: minor cleanup

6c97a30

indev29 force-pushed the feat-2588-script-module-formatting branch from d149962 to 6c97a30 Compare February 23, 2022 08:44

indev29 requested a review from patrick96 February 23, 2022 08:44

patrick96 approved these changes Feb 23, 2022

View reviewed changes

patrick96 merged commit f5bfbcc into polybar:master Feb 23, 2022

indev29 mentioned this pull request Mar 6, 2022

feat(script): add repeat interval for script failure and exec-if #2630

Merged

9 tasks

Equaraphanus mentioned this pull request Mar 10, 2022

[Bug]: Blinking custom/script module on continuous script failure with unchanged output #2636

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(script): add formatting for script failure #2596

feat(script): add formatting for script failure #2596

indev29 commented Feb 18, 2022

codecov bot commented Feb 20, 2022 •

edited

patrick96 left a comment

patrick96 Feb 20, 2022

indev29 Feb 20, 2022

patrick96 Feb 20, 2022

patrick96 left a comment

patrick96 left a comment

patrick96 commented Feb 23, 2022

feat(script): add formatting for script failure #2596

feat(script): add formatting for script failure #2596

Conversation

indev29 commented Feb 18, 2022

What type of PR is this? (check all applicable)

Description

Related Issues & Documents

Documentation (check all applicable)

codecov bot commented Feb 20, 2022 • edited

Codecov Report

patrick96 left a comment

Choose a reason for hiding this comment

patrick96 Feb 20, 2022

Choose a reason for hiding this comment

indev29 Feb 20, 2022

Choose a reason for hiding this comment

patrick96 Feb 20, 2022

Choose a reason for hiding this comment

patrick96 left a comment

Choose a reason for hiding this comment

patrick96 left a comment

Choose a reason for hiding this comment

patrick96 commented Feb 23, 2022

codecov bot commented Feb 20, 2022 •

edited