Conversation
Contributor
Reviewer's GuideAdds a new BothToNull option to RunAndLog/RunAndParse-style output handling, centralizes notice-type regex use, and makes the error notice type argument optional so callers can suppress error logging, then updates existing scripts to use the new options and calling convention. Sequence diagram for RunAndLog with BothToNull and optional error noticesequenceDiagram
actor UserScript
participant RunAndLog
participant ShellCommand as Command
participant Logger
UserScript->>RunAndLog: RunAndLog "" BothToNull "" "" pushd SCRIPTPATH
RunAndLog->>RunAndLog: Parse args and set
RunAndLog->>RunAndLog: ErrToNull=true
RunAndLog->>RunAndLog: OutToNull=true
RunAndLog->>ShellCommand: Execute pushd SCRIPTPATH
activate ShellCommand
ShellCommand-->>RunAndLog: Non-zero exit code
deactivate ShellCommand
RunAndLog->>RunAndLog: Detect failure
RunAndLog->>RunAndLog: Check ErrorNoticeType (empty)
RunAndLog->>RunAndLog: Skip error logging
RunAndLog-->>UserScript: Return failure code without logging
Note over UserScript,RunAndLog: In check_repo and check_templates_repo
UserScript->>RunAndLog: RunAndLog info git:info "" "" git -C path rev-parse
RunAndLog->>ShellCommand: Execute git command
ShellCommand-->>RunAndLog: Exit code
RunAndLog-->>UserScript: Result with optional info logging
Class-style diagram for RunAndLog usage and new BothToNull optionclassDiagram
class RunAndLog {
+RunAndLog(RunningNoticeType, OutputNoticeType, ErrorNoticeType, ErrorMessage, Command...)
-NoticeTypes_Regex
-Prefix
-ErrToNull
-OutToNull
-OutputFile
}
class update_self {
+update_self()
-calls_RunAndLog_with_BothToNull()
}
class commands_update_self_logic {
+commands_update_self_logic(Command, Notice)
-calls_RunAndLog_with_BothToNull()
}
class update_templates {
+update_templates()
-calls_RunAndLog_with_BothToNull()
}
class commands_update_templates {
+commands_update_templates(Command, Notice)
-calls_RunAndLog_with_BothToNull()
}
class main_check_repo {
+check_repo()
-calls_RunAndLog_info_git_info_without_error_notice()
}
class main_check_templates_repo {
+check_templates_repo()
-calls_RunAndLog_info_git_info_without_error_notice()
}
RunAndLog <.. update_self : uses
RunAndLog <.. commands_update_self_logic : uses
RunAndLog <.. update_templates : uses
RunAndLog <.. commands_update_templates : uses
RunAndLog <.. main_check_repo : uses
RunAndLog <.. main_check_templates_repo : uses
class OutputNoticeOptions {
+info
+notice
+warn
+error
+debug
+trace
+ErrToNull
+OutToNull
+BothToNull
}
RunAndLog --> OutputNoticeOptions : interprets
class ErrorNoticeTypeOption {
+fatal
+warn
+empty_allowed_to_suppress_logging
}
RunAndLog --> ErrorNoticeTypeOption : optional_argument
Flow diagram for updated RunAndLog output and error handlingflowchart TD
A[RunAndLog called] --> B[Read parameters
RunningNoticeType
OutputNoticeType
ErrorNoticeType
ErrorMessage
Command]
B --> C{OutputNoticeType
contains colon?}
C -- yes --> C1[Set Prefix to
substring before colon]
C -- no --> C2[Prefix is empty]
C1 --> D[Build CommandText
Prefix command args]
C2 --> D
D --> E[Initialize
ErrToNull=false
OutToNull=false]
E --> F{OutputNoticeType
matches errtonull
or bothtonull?}
F -- yes --> F1[Set ErrToNull=true]
F -- no --> G
F1 --> G{OutputNoticeType
matches outtonull
or bothtonull?}
E --> G
G -- yes --> G1[Set OutToNull=true]
G -- no --> H
G1 --> H{ErrToNull != true
or OutToNull != true
and OutputNoticeType
matches NoticeTypes_Regex}
F --> H
H -- yes --> H1[Create OutputFile
with mktemp]
H -- no --> I[No OutputFile]
H1 --> I
I --> J{ErrToNull == true
and OutToNull == true?}
J -- yes --> J1[Run Command
stdout and stderr
redirected to /dev/null]
J -- no --> K{ErrToNull == true
and OutputFile set?}
K -- yes --> K1[Run Command
stdout -> OutputFile
stderr -> /dev/null]
K -- no --> L{OutToNull == true
and OutputFile set?}
L -- yes --> L1[Run Command
stderr -> OutputFile
stdout -> /dev/null]
L -- no --> M{OutputFile set?}
M -- yes --> M1[Run Command
stdout and stderr
-> OutputFile]
M -- no --> M2[Run Command
stdout and stderr
unchanged]
J1 --> N[Capture exit code
in result]
K1 --> N
L1 --> N
M1 --> N
M2 --> N
N --> O{result == 0?}
O -- yes --> P[Return success]
O -- no --> Q{ErrorNoticeType
is non-empty?}
Q -- yes --> Q1[Call ErrorNoticeType
with ErrorMessage and
Failing command text]
Q -- no --> Q2[Skip error logging]
Q1 --> R[Return failure code]
Q2 --> R
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
errtonull|bothtonullchecks are case-sensitive, so calls likeRunAndLog "" BothToNullwill not trigger the intended null redirection; either normalizeOutputNoticeTypeto lowercase or update the regex to handle case-insensitive matching. NoticeTypes_Regexis currently a global variable insideRunAndLog; consider declaring itlocalto avoid leaking it into the broader shell environment and accidentally impacting other code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `errtonull|bothtonull` checks are case-sensitive, so calls like `RunAndLog "" BothToNull` will not trigger the intended null redirection; either normalize `OutputNoticeType` to lowercase or update the regex to handle case-insensitive matching.
- `NoticeTypes_Regex` is currently a global variable inside `RunAndLog`; consider declaring it `local` to avoid leaking it into the broader shell environment and accidentally impacting other code.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai resolve |
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
1 participant
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 support for suppressing both stdout and stderr in RunAndLog and allow calls without an error notice handler.
Enhancements: