-
Notifications
You must be signed in to change notification settings - Fork 16
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
Allow setting maxFileAdd via env var #295
Conversation
…/source-tracking into prerelease/max-file-add-test
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.
no objections. But don't merge until you fix the pjson version.
Also, recommend writing the release notes and making sure the env gets into the main list at https://developer.salesforce.com/docs/atlas.en-us.sfdx_setup.meta/sfdx_setup/sfdx_dev_cli_env_variables.htm
Co-authored-by: Juliet Shackell <[email protected]>
src/shared/localShadowRepo.ts
Outdated
this.maxFileAdd = this.isWindows ? 8000 : 15000; | ||
|
||
const batchSize = env.getNumber('SFDX_SOURCE_TRACKING_BATCH_SIZE'); | ||
this.maxFileAdd = batchSize ? batchSize : this.isWindows ? 8000 : 15000; |
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.
rather than nested ternary, you could do something like
this.maxFileAdd = batchSize ? batchSize : this.isWindows ? 8000 : 15000; | |
this.maxFileAdd = batchSize ?? this.isWindows ? 8000 : 15000; |
or just
this.maxFileAdd = env.getNumber('SFDX_SOURCE_TRACKING_BATCH_SIZE') ?? this.isWindows ? 8000 : 15000
or use the "default" 2nd prop of env,
this.maxFileAdd = env.getNumber('SFDX_SOURCE_TRACKING_BATCH_SIZE', this.isWindows ? 8000 : 15000)
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.
Yea, I generally don't like nested ternaries but I like the combination of a nullish coalescing and a ternary less. Mainly because it is not immediately obvious what is being compared. The code you suggested would not work correctly, it would compare the nullish first and use that result for the ternary. If you want to combine the two operators, you'd need to wrap your ternary in parentheses.
// resolves 'tern-true' 👎
console.log('foo' ?? true ? 'tern-true' : 'tern-false');
// resolves 'foo'
console.log('foo' ?? (true ? 'tern-true' : 'tern-false'));
// resolves ""
console.log('' ?? (true ? 'tern-true' : 'tern-false'));
// resolves "tern-true"
console.log(undefined ?? (true ? 'tern-true' : 'tern-false'));
// resolves "tern-false"
console.log(undefined ?? (false ? 'tern-true' : 'tern-false'));
https://jsfiddle.net/hxjzaq5c/1/
I do like the idea of using the default for getNumber
, I will switch to that. Thanks for the suggestion
What does this PR do?
Lowers maxFileAdd for non-windows oses
What issues does this PR fix or reference?
@W-12256168@