Suggested improvements for scripts/lb-wrapper [refactor logging & subprocess substitution, etc] #87
Conversation
|
Example test results — after running the same/above test using the Download to IIAB button: |
|
Just FYI this PR also:
|
| else | ||
| log "An error occurred while running the command. Output: ${OUTPUT}" | ||
| log "Error" "Error $rc occurred while running lb-wrapper's xklb commands." |
There was a problem hiding this comment.
Good you log the return code here.
Co-authored-by: Blondel MONDESIR <[email protected]>
|
@deldesir can you confirm the ~50+ "Errors" (pasted in "Example output" above) are ugly-but-acceptable for now? i.e. if giving more visibility to these minor errors from xklb and yt-dlp and FFmpeg etc is a central purpose of this PR: ➡️ But should we perhaps relabel Line 64 with |
scripts/lb-wrapper
Outdated
| # 1>&2 redirect back-to-STDERR explained at https://stackoverflow.com/a/15936384 | ||
| eval "${XKLB_FULL_CMD}" \ | ||
| > >(while read -r line; do log "Info" "$line"; done) \ | ||
| 2> >(while read -r line; do log "Error" "$line" 1>&2; done) & |
There was a problem hiding this comment.
| 2> >(while read -r line; do log "Error" "$line" 1>&2; done) & | |
| 2> >(while read -r line; do log "Warning" "$line" 1>&2; done) & |
You should have noticed a couple of messages in the output that are tagged as errors, but they are related to the normal xklb execution. These messages shouldn't impact the functionality of lb-wrapper; they are just part of the informative output of xklb printed in STDERR instead of STDOUT. |
|
Good Enough: PR #86 introduced most of the ideas here (let's give credit!) but grew somewhat too large to understand. @deldesir and @EMG70 please put additional recommendations in new tickets or PR's to further improve on lb-wrapper ✅ So we keep TDD momentum with regular, small, clearly explained PR's, Thank You! ASIDE: This PR #87 can also serve to gently push us all to do the Earnest Hard Work of breaking apart larger ideas (large PR's) into smaller ideas (small PR's) — in order to build a Stronger Collaboration Culture upholding IIAB communities in all countries:
|
Some prefer the word "Debug" as it's shorter than "Warning" and somewhat more precise! Either is fine for me, but brevity's better possibly? |
|
"Debug" aligns more with them IMHO. They are informative messages. |
Done! As part this separate PR: |
holta
changed the title
|
Sorry for missing this earlier,
Agree that error seems to be a misnomer here and should be reserved for more urgent problems. Since this is only for informative purposes, I agree with @deldesir that "Debug" makes sense. "Warning" would be used for something wasn't working as expected, imo. |
An executive decision was made to replace PR #86 with smaller PR's. Hopefully this (or similar) is one of those PR's.
This PR was tested on a Ubuntu 24.04 pre-release VM. And is already getting too large.
Thanks @deldesir and @EMG70 for reviewing. I think it's good enough until @deldesir and others improve on it later. But LMK your thoughts, with any questions/clarifications that are needed.
Example output
Related:
YouTube_Movies_TVchannel encrypted w/ browser-based DRM?] #85