-
Notifications
You must be signed in to change notification settings - Fork 1
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
Update forkrun.bash #52
base: forkrun_testing
Are you sure you want to change the base?
Conversation
consolidate variables for flags and file descriptors into two bash associative arrays (`flagsA` and `fdA`)
Reviewer's Guide by SourceryThe changes consolidate flag and file descriptor variables into two bash associative arrays ( Class diagram for associative arrays
|
Change | Details | Files |
---|---|---|
Introduced two bash associative arrays to consolidate variables |
|
forkrun.bash |
Migrated flag variables to flagsA array |
|
forkrun.bash |
Migrated file descriptor variables to fdA array |
|
forkrun.bash |
Updated code references to use the new array structure |
|
forkrun.bash |
Tips and commands
Interacting with Sourcery
- Trigger a new review: Comment
@sourcery-ai review
on the pull request. - Continue discussions: Reply directly to Sourcery's review comments.
- Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. - Generate a pull request title: Write
@sourcery-ai
anywhere in the pull
request title to generate a title at any time. - Generate a pull request summary: Write
@sourcery-ai summary
anywhere in
the pull request body to generate a PR summary at any time. You can also use
this command to specify where the summary should be inserted.
Customizing Your Experience
Access your dashboard to:
- Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others. - Change the review language.
- Add, remove or edit custom review instructions.
- Adjust other review settings.
Getting Help
- Contact our support team for questions or feedback.
- Visit our documentation for detailed guides and information.
- Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jkool702 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding documentation comments explaining the purpose and structure of the new flagsA and fdA associative arrays
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
local -i PID0 nLines nLinesCur nLinesNew nLinesMax nRead nWait nOrder0 nBytesRead nQueue nQueueLast nQueueMin nQueueLastCount nCPU v9 kkMax kkCur kk kkProcs verboseLevel pLOAD_max pAdd | ||
local -a A p_PID runCmd outHave outPrint pLOADA | ||
local -A flagsA fdA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider simplifying the flag handling by using either scoped boolean variables or a more concise array access pattern.
The change to use an associative array for flags has made the code more complex and harder to read. While grouping related flags can be beneficial, the current implementation with verbose ${flagsA['key']}
syntax scattered throughout adds unnecessary cognitive overhead.
Consider one of these cleaner approaches:
- Using properly scoped boolean variables with a clear naming convention:
local -b flag_nLinesAuto=false
local -b flag_readBytes=false
- Or if using an array, use a more concise access pattern:
# At the start, create local references
local -n f=flagsA # Create a reference
: "${f[nLinesAuto]:=false}"
: "${f[readBytes]:=false}"
# Then use the shorter form in the code
if ${f[nLinesAuto]}; then
...
fi
This maintains the benefits of grouping related flags while being more readable and maintainable.
consolidate variables for flags and file descriptors into two bash associative arrays (
flagsA
andfdA
)Summary by Sourcery
Enhancements: