Update forkrun.bash #53
Open
Conversation
This file contains 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
Reviewer's Guide by SourceryThe changes focus on improving code readability and functionality in the forkrun.bash script. The main changes include reorganizing variable declarations, enhancing the number parsing functionality through the _forkrun_getVal function, and fixing indentation consistency throughout the code. Class diagram for updated variable declarations in forkrun.bashclassDiagram
class Forkrun {
- tmpDir
- fPath
- outStr
- delimiterVal
- delimiterReadStr
- delimiterRemoveStr
- exitTrapStr
- exitTrapStr_kill
- nLines
- nLines0
- nLinesMax
- nQueueMin
- nOrder
- nProcs
- nProcsMax
- nBytes
- tTimeout
- coprocSrcCode
- outCur
- tmpDirRoot
- returnVal
- tmpVar
- t0
- readBytesProg
- nullDelimiterProg
- ddQuietStr
- trailingNullFlag
- inotifyFlag
- lseekFlag
- fallocateFlag
- nLinesAutoFlag
- nQueueFlag
- substituteStringFlag
- substituteStringIDFlag
- nOrderFlag
- readBytesFlag
- readBytesExactFlag
- nullDelimiterFlag
- subshellRunFlag
- stdinRunFlag
- pipeReadFlag
- rmTmpDirFlag
- exportOrderFlag
- noFuncFlag
- unescapeFlag
- optParseFlag
- continueFlag
- doneIndicatorFlag
- FORCE_allowCarriageReturnsFlag
- ddAvailableFlag
- fd_continue
- fd_inotify
- fd_inotify0
- fd_nAuto
- fd_nAuto0
- fd_nOrder
- fd_nOrder0
- fd_read
- fd_read0
- fd_write
- fd_stdout
- fd_stdin
- fd_stdin0
- fd_stderr
- pWrite
- pOrder
- pAuto
- pQueue
- pWrite_PID
- pNotify_PID
- pOrder_PID
- pAuto_PID
- pQueue_PID
- fd_read_pos
- fd_read_pos_old
- fd_write_pos
- DEBUG_FORKRUN
- PID0
- nLinesCur
- nLinesNew
- nRead
- nWait
- nOrder0
- nBytesRead
- nQueue
- nQueueLast
- nQueueLastCount
- nCPU
- v9
- kkMax
- kkCur
- kk
- kkProcs
- verboseLevel
- pLOAD_max
- pAdd
- A
- p_PID
- runCmd
- outHave
- outPrint
- pLOADA
- pMap
}
Class diagram for _forkrun_getVal functionclassDiagram
class Forkrun {
+ _forkrun_getVal()
}
note for Forkrun "Enhances number parsing functionality by expanding IEC and SI prefixes to numeric values."
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}
There was a problem hiding this comment.
Hey @jkool702 - I've reviewed your changes - here's some feedback:
Overall Comments:
- The commented out 'local -AI pMap' declaration should probably be kept to ensure pMap remains an associative integer array
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 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.
forkrun.bash
Outdated
| nn="${nn%i}"; | ||
|
|
||
| case "${nn:0:1}" in | ||
| '+') | ||
| echo "$(( ${nn//[^0-9]/} * ( 1000 ** ${pMap[${nn: -1}]:-0} ) ))" | ||
| echo "$(( ${nn//[^0-9]/} * ( 1000 ** ${pMap[${nn:'-1'}]:-0} ) ))" |
There was a problem hiding this comment.
issue (bug_risk): The string slicing syntax ${nn:'-1'} is incorrect - the quotes will prevent it from actually slicing the string
This should be ${nn:-1} for string slicing. The current syntax treats '-1' as a default value rather than a slice index.
forkrun.bash
Outdated
|
|
||
| local +i nn | ||
| local -AI pMap | ||
| #local -AI pMap |
There was a problem hiding this comment.
issue (bug_risk): The pMap array declaration is commented out but the array is used throughout the function
Either remove this line entirely if pMap is declared elsewhere, or uncomment it if it's needed here. The current state is ambiguous and could lead to bugs.
| @@ -1859,31 +1859,33 @@ _forkrun_get_load() ( | |||
|
|
|||
| _forkrun_getVal() { | |||
There was a problem hiding this comment.
issue (complexity): Consider simplifying prefix handling by using a lookup table with pre-computed values instead of runtime calculations
The prefix handling can be simplified using a lookup table approach while preserving all functionality. Here's a suggested implementation:
_forkrun_getVal() {
local -A multipliers=(
[k]=1024 [kb]=1024 [kib]=1024
[m]=1048576 [mb]=1048576 [mib]=1048576
[+k]=1000 [+kb]=1000 [+kib]=1024
[+m]=1000000 [+mb]=1000000 [+mib]=1048576
# Add other prefixes similarly...
)
local nn
for nn in "${@,,}"; do
nn="${nn%[b]}"
local base="${nn//[0-9]/}"
local num="${nn//[^0-9]/}"
echo "$((num * ${multipliers[$base]:-1}))"
done
}This approach:
- Uses a single lookup table with pre-computed values
- Eliminates complex string manipulations and case statements
- Preserves all functionality including IEC/SI prefix handling
- Maintains backward compatibility
- Makes the behavior more explicit and easier to maintain
apply _forkrun_getVal to all user-provided numeric options so that any user-specified number can use a SI/IEC prefix
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @jkool702 - I've reviewed your changes and they look great!
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.
| @@ -1858,32 +1850,29 @@ _forkrun_get_load() ( | |||
| ) | |||
|
|
|||
| _forkrun_getVal() { | |||
There was a problem hiding this comment.
issue (complexity): Consider simplifying the _forkrun_getVal function by using a more structured approach with clear variable naming and separation of concerns.
The _forkrun_getVal function has become unnecessarily complex. Here's a cleaner approach that maintains all functionality:
_forkrun_getVal() {
# Handles both IEC (1024^N) and SI (1000^N) prefixes
# Examples: 1K=1000, 1KiB=1024, +1K=1024
local -l input # Force lowercase
local base multiplier prefix
for input in "${@%%[Bb]*}"; do # Strip trailing B/b
input="${input// /}" # Remove spaces
# Extract numeric part and prefix
local num="${input//[^0-9]/}"
prefix="${input##*[0-9]}"
# Determine base (1000 vs 1024)
if [[ "$input" == *i* ]] || [[ "$input" == +* ]]; then
base=1024
multiplier=${pMap[${prefix%i}]:-0} # Strip 'i' from prefix
else
base=1000
multiplier=${pMap[${prefix}]:-0}
fi
# Calculate and output
if ((base == 1024)); then
printf '%s\n' "$((num << (10 * multiplier)))"
else
printf '%s\n' "$((num * (base ** multiplier)))"
fi
done
}This version:
- Clearly separates IEC vs SI prefix handling
- Uses descriptive variable names
- Has a more logical flow with input normalization first
- Maintains all edge case handling
- Uses simpler pattern matching
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.
Summary by Sourcery
Enhancements: