Skip to content

Commit 80ca406

Browse files
jfeingold35jfeingold35
authored
Merge pull request #2554 from jon-freed/fix/flags-dir-cli-override-precedence
fix: use || instead of ?? for CLI flag override precedence in preparse hook @W-20991305@
2 parents 5628c27 + d7ec648 commit 80ca406

2 files changed

Lines changed: 290 additions & 15 deletions

File tree

‎src/hooks/preparse.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,12 @@ const hook: Hook.Preparse = async function ({ argv, options, context }) {
4646
)
4747
.filter(
4848
([flagName, flagOptions]) =>
49-
// ignore if short char flag is present
50-
(flagOptions.char && argv.includes(`-${flagOptions.char}`)) ??
49+
// Using || instead of ?? is intentional here: argv.includes() returns false (not null/undefined)
50+
// when the flag isn't found, and we need false to trigger evaluation of subsequent conditions.
51+
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
52+
(flagOptions.char && argv.includes(`-${flagOptions.char}`)) ||
5153
// ignore if long flag is present
52-
argv.includes(`--${flagName}`) ??
54+
argv.includes(`--${flagName}`) ||
5355
// ignore if --no- flag is present
5456
(flagOptions.type === 'boolean' && flagOptions.allowNo && argv.includes(`--no-${flagName}`))
5557
)

‎test/hooks/preparse.test.ts‎

Lines changed: 285 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -184,18 +184,6 @@ describe('preparse hook test', () => {
184184
expect(results.successes[0].result).to.deep.equal([...baseArgs, '--str', 'value']);
185185
});
186186

187-
it('should add string from directory and allow short char flag overrides', async () => {
188-
const argv = [...baseArgs, '-s', 'value'];
189-
const flags = {
190-
...baseFlags,
191-
str: Flags.string({ char: 's' }),
192-
};
193-
makeStubs([{ name: 'str', content: 'value' }]);
194-
const results = await config.runHook('preparse', { argv, options: { flags } });
195-
expect(results.successes[0]).to.be.ok;
196-
expect(results.successes[0].result).to.deep.equal([...baseArgs, '-s', 'value']);
197-
});
198-
199187
it('should handle single line json string', async () => {
200188
const argv = [...baseArgs];
201189
const flags = {
@@ -410,4 +398,289 @@ describe('preparse hook test', () => {
410398
expect(results.successes[0].result).to.deep.equal([...baseArgs, '--str', ' value with spaces ']);
411399
});
412400
});
401+
402+
// ============================================================================
403+
// CLI OVERRIDE PRECEDENCE TESTS
404+
// ============================================================================
405+
//
406+
// WHAT WE'RE TESTING:
407+
// When a user specifies a flag on the CLI AND the same flag exists as a file
408+
// in --flags-dir, what happens?
409+
// Per PR #1536 (https://github.com/salesforcecli/cli/pull/1536):
410+
// "Flags/values provided by the user will override any flag/values found by
411+
// --flags-dir -- unless the flag supports `multiple`, in which case they will
412+
// all be combined."
413+
//
414+
// HOW FLAGS-DIR WORKS:
415+
// - Files in flags-dir may be named with the LONG flag name (e.g., "test-level")
416+
// - File contents become the flag value
417+
// - The hook inserts these as argv entries AFTER the user's CLI args
418+
//
419+
// THE OVERRIDE CHECK:
420+
// Before inserting a flags-dir value, the hook checks if that flag was already
421+
// provided on CLI. The check must detect BOTH CLI forms:
422+
// - Short/char form: -l RunLocalTests
423+
// - Long form: --test-level RunLocalTests
424+
// If either form is found on CLI, the flags-dir file is skipped (for single-
425+
// value flags). For multiple-value flags, both CLI and flags-dir are combined.
426+
//
427+
// WHY ONLY LONG-FORM FILENAMES IN FLAGS-DIR:
428+
// The override check uses long flag names (e.g., "test-level") to match files.
429+
// A file named "l" would NOT be recognized as the same flag, so it would always
430+
// be inserted regardless of what's provided on the CLI. These tests use
431+
// long-named files only.
432+
//
433+
// FLAG TYPES:
434+
//
435+
// Type | Description | Real example | Test flag
436+
// ------------------|----------------------|--------------------------|-------------------------
437+
// String (single) | Takes one value | --test-level RunLocal | --myflag my-cli-val
438+
// String (multiple) | Takes multiple vals | --source-dir src force | --myflag my-cli-val
439+
// Boolean | Present or absent | --dry-run | --myflag
440+
// Boolean (allowNo) | Can be negated | --wait or --no-wait | --myflag or --no-myflag
441+
//
442+
// A flag with a "short/char" form has a short, single character version, (e.g. 'm').
443+
// Real example: -l is the short/char for --test-level. Our tests use -m for --myflag.
444+
//
445+
// TEST MATRIX:
446+
//
447+
// # | Flag Type | short/char version? | flags-dir file (contents) | CLI argument | Result
448+
// ---|-------------------|----------------------|------------------------------|----------------------|---------------
449+
// 1 | String (single) | no | myflag (my-flags-dir-val) | --myflag my-cli-val | CLI overrides
450+
// 2 | String (single) | yes (and uses it) | myflag (my-flags-dir-val) | -m my-cli-val | CLI overrides
451+
// 3 | String (single) | yes (doesn't use it) | myflag (my-flags-dir-val) | --myflag my-cli-val | CLI overrides *
452+
// 4 | String (multiple) | no | myflag (my-flags-dir-val) | --myflag my-cli-val | Combined
453+
// 5 | String (multiple) | yes (and uses it) | myflag (my-flags-dir-val) | -m my-cli-val | Combined
454+
// 6 | String (multiple) | yes (doesn't use it) | myflag (my-flags-dir-val) | --myflag my-cli-val | Combined
455+
// 7 | Boolean | no | myflag (empty) | --myflag | CLI overrides
456+
// 8 | Boolean | yes (and uses it) | myflag (empty) | -m | CLI overrides
457+
// 9 | Boolean | yes (doesn't use it) | myflag (empty) | --myflag | CLI overrides *
458+
// 10 | Boolean (allowNo) | no | no-myflag (empty) | --myflag | CLI overrides
459+
// 11 | Boolean (allowNo) | no | myflag (empty) | --no-myflag | CLI overrides *
460+
// 12 | Boolean (allowNo) | yes (and uses it) | no-myflag (empty) | -m | CLI overrides
461+
// 13 | Boolean (allowNo) | yes (doesn't use it) | no-myflag (empty) | --myflag | CLI overrides *
462+
// 14 | Boolean (allowNo) | yes (doesn't use it) | myflag (empty) | --no-myflag | CLI overrides *
463+
//
464+
// * = Would have failed with bug from commit c83f81a (used ?? instead of ||)
465+
// The bug affected cases where a flag HAS a char defined, but the user
466+
// typed the long form on CLI (e.g., --myflag instead of -m).
467+
//
468+
// ============================================================================
469+
470+
describe('CLI override precedence tests', () => {
471+
describe('String flags (single value) - CLI should override flags-dir', () => {
472+
it('should override flags-dir value with CLI value (#1: Flag is for String (single), no short/char version)', async () => {
473+
const argv = [...baseArgs, '--myflag', 'my-cli-val'];
474+
const flags = {
475+
...baseFlags,
476+
myflag: Flags.string(), // No char defined
477+
};
478+
makeStubs([{ name: 'myflag', content: 'my-flags-dir-val' }]);
479+
const results = await config.runHook('preparse', { argv, options: { flags } });
480+
expect(results.successes[0]).to.be.ok;
481+
expect(results.successes[0].result).to.deep.equal([...baseArgs, '--myflag', 'my-cli-val']);
482+
});
483+
484+
it('should override flags-dir value with CLI value (#2: Flag is for String (single), has short/char version (and uses it))', async () => {
485+
const argv = [...baseArgs, '-m', 'my-cli-val'];
486+
const flags = {
487+
...baseFlags,
488+
myflag: Flags.string({ char: 'm' }),
489+
};
490+
makeStubs([{ name: 'myflag', content: 'my-flags-dir-val' }]);
491+
const results = await config.runHook('preparse', { argv, options: { flags } });
492+
expect(results.successes[0]).to.be.ok;
493+
// CLI value should win - flags-dir value should NOT be added
494+
expect(results.successes[0].result).to.deep.equal([...baseArgs, '-m', 'my-cli-val']);
495+
});
496+
497+
it("should override flags-dir value with CLI value (#3: Flag is for String (single), has short/char version (doesn't use it))", async () => {
498+
// (commit c83f81a's bug causes this test to fail)
499+
const argv = [...baseArgs, '--myflag', 'my-cli-val'];
500+
const flags = {
501+
...baseFlags,
502+
myflag: Flags.string({ char: 'm' }), // Has char, but CLI uses --myflag not -m
503+
};
504+
makeStubs([{ name: 'myflag', content: 'my-flags-dir-val' }]);
505+
const results = await config.runHook('preparse', { argv, options: { flags } });
506+
expect(results.successes[0]).to.be.ok;
507+
// CLI value should win - flags-dir value should NOT be added
508+
// With bug: would be [...baseArgs, '--myflag', 'my-cli-val', '--myflag', 'my-flags-dir-val']
509+
expect(results.successes[0].result).to.deep.equal([...baseArgs, '--myflag', 'my-cli-val']);
510+
});
511+
});
512+
513+
describe('String flags (multiple: true) - CLI and flags-dir should combine', () => {
514+
it('should combine flags-dir with CLI value (#4: Flag is for String (multiple), no short/char version)', async () => {
515+
const argv = [...baseArgs, '--myflag', 'my-cli-val'];
516+
const flags = {
517+
...baseFlags,
518+
myflag: Flags.string({ multiple: true }), // No char defined
519+
};
520+
makeStubs([{ name: 'myflag', content: 'my-flags-dir-val' }]);
521+
const results = await config.runHook('preparse', { argv, options: { flags } });
522+
expect(results.successes[0]).to.be.ok;
523+
// Both values should be present (combined)
524+
expect(results.successes[0].result).to.deep.equal([
525+
...baseArgs,
526+
'--myflag',
527+
'my-cli-val',
528+
'--myflag',
529+
'my-flags-dir-val',
530+
]);
531+
});
532+
533+
it('should combine flags-dir with CLI value (#5: Flag is for String (multiple), has short/char version (and uses it))', async () => {
534+
const argv = [...baseArgs, '-m', 'my-cli-val'];
535+
const flags = {
536+
...baseFlags,
537+
myflag: Flags.string({ multiple: true, char: 'm' }),
538+
};
539+
makeStubs([{ name: 'myflag', content: 'my-flags-dir-val' }]);
540+
const results = await config.runHook('preparse', { argv, options: { flags } });
541+
expect(results.successes[0]).to.be.ok;
542+
// Both values should be present (combined)
543+
expect(results.successes[0].result).to.deep.equal([
544+
...baseArgs,
545+
'-m',
546+
'my-cli-val',
547+
'--myflag',
548+
'my-flags-dir-val',
549+
]);
550+
});
551+
552+
it("should combine flags-dir with CLI value (#6: Flag is for String (multiple), has short/char version (doesn't use it))", async () => {
553+
const argv = [...baseArgs, '--myflag', 'my-cli-val'];
554+
const flags = {
555+
...baseFlags,
556+
myflag: Flags.string({ multiple: true, char: 'm' }),
557+
};
558+
makeStubs([{ name: 'myflag', content: 'my-flags-dir-val' }]);
559+
const results = await config.runHook('preparse', { argv, options: { flags } });
560+
expect(results.successes[0]).to.be.ok;
561+
// Both values should be present (combined)
562+
expect(results.successes[0].result).to.deep.equal([
563+
...baseArgs,
564+
'--myflag',
565+
'my-cli-val',
566+
'--myflag',
567+
'my-flags-dir-val',
568+
]);
569+
});
570+
});
571+
572+
describe('Boolean flags - CLI should override flags-dir', () => {
573+
it('should override flags-dir value with CLI value (#7: Flag is for Boolean, no short/char version)', async () => {
574+
const argv = [...baseArgs, '--myflag'];
575+
const flags = {
576+
...baseFlags,
577+
myflag: Flags.boolean(), // No char defined
578+
};
579+
makeStubs([{ name: 'myflag', content: '' }]); // Empty file for boolean
580+
const results = await config.runHook('preparse', { argv, options: { flags } });
581+
expect(results.successes[0]).to.be.ok;
582+
// CLI should win - flags-dir should NOT add duplicate
583+
expect(results.successes[0].result).to.deep.equal([...baseArgs, '--myflag']);
584+
});
585+
586+
it('should override flags-dir value with CLI value (#8: Flag is for Boolean, has short/char version (and uses it))', async () => {
587+
const argv = [...baseArgs, '-m'];
588+
const flags = {
589+
...baseFlags,
590+
myflag: Flags.boolean({ char: 'm' }),
591+
};
592+
makeStubs([{ name: 'myflag', content: '' }]);
593+
const results = await config.runHook('preparse', { argv, options: { flags } });
594+
expect(results.successes[0]).to.be.ok;
595+
// CLI should win - flags-dir should NOT add duplicate
596+
expect(results.successes[0].result).to.deep.equal([...baseArgs, '-m']);
597+
});
598+
599+
it("should override flags-dir value with CLI value (#9: Flag is for Boolean, has short/char version (doesn't use it))", async () => {
600+
// (commit c83f81a's bug causes this test to fail)
601+
const argv = [...baseArgs, '--myflag'];
602+
const flags = {
603+
...baseFlags,
604+
myflag: Flags.boolean({ char: 'm' }), // Has char, but CLI uses --myflag not -m
605+
};
606+
makeStubs([{ name: 'myflag', content: '' }]);
607+
const results = await config.runHook('preparse', { argv, options: { flags } });
608+
expect(results.successes[0]).to.be.ok;
609+
// CLI should win - flags-dir should NOT add duplicate
610+
// With bug: would be [...baseArgs, '--myflag', '--myflag']
611+
expect(results.successes[0].result).to.deep.equal([...baseArgs, '--myflag']);
612+
});
613+
});
614+
615+
describe('Boolean flags (allowNo: true) - CLI should override flags-dir', () => {
616+
it('should override flags-dir value with CLI value (#10: Flag is for Boolean (allowNo), no short/char version)', async () => {
617+
const argv = [...baseArgs, '--myflag'];
618+
const flags = {
619+
...baseFlags,
620+
myflag: Flags.boolean({ allowNo: true }), // No char
621+
};
622+
makeStubs([{ name: 'no-myflag', content: '' }]); // Negated file
623+
const results = await config.runHook('preparse', { argv, options: { flags } });
624+
expect(results.successes[0]).to.be.ok;
625+
// CLI --myflag should win, flags-dir no-myflag should NOT be added
626+
expect(results.successes[0].result).to.deep.equal([...baseArgs, '--myflag']);
627+
});
628+
629+
it('should override flags-dir value with CLI value (#11: Flag is for Boolean (allowNo), no short/char version, --no-myflag)', async () => {
630+
// (commit c83f81a's bug causes this test to fail)
631+
const argv = [...baseArgs, '--no-myflag'];
632+
const flags = {
633+
...baseFlags,
634+
myflag: Flags.boolean({ allowNo: true }), // No char
635+
};
636+
makeStubs([{ name: 'myflag', content: '' }]); // Positive file
637+
const results = await config.runHook('preparse', { argv, options: { flags } });
638+
expect(results.successes[0]).to.be.ok;
639+
// CLI --no-myflag should win, flags-dir myflag should NOT be added
640+
expect(results.successes[0].result).to.deep.equal([...baseArgs, '--no-myflag']);
641+
});
642+
643+
it('should override flags-dir value with CLI value (#12: Flag is for Boolean (allowNo), has short/char version (and uses it))', async () => {
644+
const argv = [...baseArgs, '-m'];
645+
const flags = {
646+
...baseFlags,
647+
myflag: Flags.boolean({ allowNo: true, char: 'm' }),
648+
};
649+
makeStubs([{ name: 'no-myflag', content: '' }]);
650+
const results = await config.runHook('preparse', { argv, options: { flags } });
651+
expect(results.successes[0]).to.be.ok;
652+
// CLI -m should win, flags-dir no-myflag should NOT be added
653+
expect(results.successes[0].result).to.deep.equal([...baseArgs, '-m']);
654+
});
655+
656+
it("should override flags-dir value with CLI value (#13: Flag is for Boolean (allowNo), has short/char version (doesn't use it), --myflag)", async () => {
657+
// (commit c83f81a's bug causes this test to fail)
658+
const argv = [...baseArgs, '--myflag'];
659+
const flags = {
660+
...baseFlags,
661+
myflag: Flags.boolean({ allowNo: true, char: 'm' }), // Has char, CLI uses --myflag
662+
};
663+
makeStubs([{ name: 'no-myflag', content: '' }]);
664+
const results = await config.runHook('preparse', { argv, options: { flags } });
665+
expect(results.successes[0]).to.be.ok;
666+
// CLI --myflag should win, flags-dir no-myflag should NOT be added
667+
// With bug: would be [...baseArgs, '--myflag', '--no-myflag']
668+
expect(results.successes[0].result).to.deep.equal([...baseArgs, '--myflag']);
669+
});
670+
671+
it("should override flags-dir value with CLI value (#14: Flag is for Boolean (allowNo), has short/char version (doesn't use it), --no-myflag)", async () => {
672+
const argv = [...baseArgs, '--no-myflag'];
673+
const flags = {
674+
...baseFlags,
675+
myflag: Flags.boolean({ allowNo: true, char: 'm' }), // Has char, CLI uses --no-myflag
676+
};
677+
makeStubs([{ name: 'myflag', content: '' }]);
678+
const results = await config.runHook('preparse', { argv, options: { flags } });
679+
expect(results.successes[0]).to.be.ok;
680+
// CLI --no-myflag should win, flags-dir myflag should NOT be added
681+
// With bug: would be [...baseArgs, '--no-myflag', '--myflag']
682+
expect(results.successes[0].result).to.deep.equal([...baseArgs, '--no-myflag']);
683+
});
684+
});
685+
});
413686
});

0 commit comments

Comments
 (0)