Conversation
Contributor
Reviewer's GuideAdds generalized RunAndLog and helper PrefixFileLines implementations directly into main.sh to standardize command execution, output prefixing, and logging behavior, including flexible notice types and output redirection modes. Sequence diagram for RunAndLog command execution and loggingsequenceDiagram
participant Caller
participant RunAndLog
participant SystemCommand
participant TempFile as OutputFile
participant Logger
Caller->>RunAndLog: RunAndLog(RunningNoticeType, OutputNoticeType, ErrorNoticeType, ErrorMessage, Command)
alt RunningNoticeType is set
RunAndLog->>Logger: RunningNoticeType("Running: CommandText")
end
alt OutputNoticeType requires logging and not both null
RunAndLog->>TempFile: Create temp file for command output
end
alt Both stdout and stderr to null
RunAndLog->>SystemCommand: Execute Command &> /dev/null
SystemCommand-->>RunAndLog: exit code
else stderr to null, stdout to file
RunAndLog->>SystemCommand: Execute Command > OutputFile 2> /dev/null
SystemCommand-->>RunAndLog: exit code
else stdout to null, stderr to file
RunAndLog->>SystemCommand: Execute Command 2> OutputFile > /dev/null
SystemCommand-->>RunAndLog: exit code
else stdout and stderr to file
RunAndLog->>SystemCommand: Execute Command &> OutputFile
SystemCommand-->>RunAndLog: exit code
else no redirection
RunAndLog->>SystemCommand: Execute Command
SystemCommand-->>RunAndLog: exit code
end
alt OutputFile exists and is nonempty
RunAndLog->>TempFile: Read lines
RunAndLog->>RunAndLog: PrefixFileLines(Prefix, OutputFile)
RunAndLog->>Logger: OutputNoticeType(prefixed output)
RunAndLog->>TempFile: Delete OutputFile
end
alt exit code is nonzero and ErrorNoticeType is set
RunAndLog->>Logger: ErrorNoticeType(ErrorMessage, Failing command: CommandText)
end
RunAndLog-->>Caller: Return exit code
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In the
RunAndLogtail section, instead ofif [[ -n ${OutputFile-} && -n $(cat "${OutputFile}") ]]; then, consider using[[ -n ${OutputFile-} && -s ${OutputFile} ]]to avoid a uselesscat, eliminate subshell/word-splitting concerns, and rely on the built-in file size test. - The string flags
ErrToNullandOutToNullinRunAndLogwork, but usinglocal ErrToNull=false/trueand testing them as[ "$ErrToNull" = true ](or just using separate condition branches without boolean variables) would simplify the control flow and make the redirection handling easier to read.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `RunAndLog` tail section, instead of `if [[ -n ${OutputFile-} && -n $(cat "${OutputFile}") ]]; then`, consider using `[[ -n ${OutputFile-} && -s ${OutputFile} ]]` to avoid a useless `cat`, eliminate subshell/word-splitting concerns, and rely on the built-in file size test.
- The string flags `ErrToNull` and `OutToNull` in `RunAndLog` work, but using `local ErrToNull=false`/`true` and testing them as `[ "$ErrToNull" = true ]` (or just using separate condition branches without boolean variables) would simplify the control flow and make the redirection handling easier to read.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request
Purpose
Describe the problem or feature in addition to a link to the issues.
Approach
How does this change address the problem?
Open Questions and Pre-Merge TODOs
Check all boxes as they are completed
Learning
Describe the research stage
Links to blog posts, patterns, libraries or addons used to solve this problem
Requirements
Check all boxes as they are completed
Summary by Sourcery
Add shared logging helpers to centralize command execution and output handling in main.sh.
Enhancements: