-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Refactor $WEB_CONCURRENCY handling #931
Conversation
608bafb
to
4facf41
Compare
profile/WEB_CONCURRENCY.sh
Outdated
|
||
validate_concurrency() { | ||
local concurrency=$1 | ||
local oob=0 |
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.
What does oob
mean?
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.
"out of bounds"; it's the return value (as opposed to function result, which is what gets echo
'd so you can capture it with $()
) that lets calling code determine whether the calculated (or given) was too high or too low
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.
That's why the "#FIXME
" comment is there - we could also change the warning function to warn_high_concurrency
, removing the logic that checks for > 200
(which right now is still duplicated), and only call it if oob == 2
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.
Got it now, thanks. Maybe worth adding a code comment about it? We could also eliminate the name oob
completely as it's really just the return value. local return_value
maybe? I was looking for a deeper meaning beyond "1 is lower bound exceeded and 2 is upper bound exceeded".
profile/WEB_CONCURRENCY.sh
Outdated
|
||
validate_concurrency() { | ||
local concurrency=$1 | ||
local oob=0 |
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.
Got it now, thanks. Maybe worth adding a code comment about it? We could also eliminate the name oob
completely as it's really just the return value. local return_value
maybe? I was looking for a deeper meaning beyond "1 is lower bound exceeded and 2 is upper bound exceeded".
4facf41
to
52a8d7f
Compare
Changes the various functions around so they no longer have side effects.