Skip to content
This repository has been archived by the owner on Aug 22, 2023. It is now read-only.

Handle flags with multiple inputs without the flag before each input #25

Merged
merged 6 commits into from
Jun 1, 2018

Conversation

idandrd
Copy link
Contributor

@idandrd idandrd commented May 31, 2018

What

So far the parser supported flags with multiple inputs only if the flag shows before each input,
so this worked: MyCli mycmd -i foo -i bar
but this didn't: MyCli mycmd -i foo bar

Why

When passing FS paths with wildcards, the paths are parsed by the terminal before they reach the oclif code. so if I type: MyCli readFiles -i ./files/*.txt the oclif will actually get MyCli readFiles -i ./files/a.txt ./files/b.txt ./files/c.txt and the command will fail.
I needed a way to allow my users to select multiple files in one command, so this functionality was needed.

How

I kept a copy of the last flag parsed. if the next string in argv is not a flag, and the last flag parsed is accepting multiple values, the string will be added as a value for this flag.

@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @idandrd to sign the Salesforce.com Contributor License Agreement.

src/parse.ts Outdated
@@ -47,8 +47,10 @@ export class Parser<T extends ParserInput, TFlags extends OutputFlags<T['flags']
private readonly raw: ParsingToken[] = []
private readonly booleanFlags: { [k: string]: Flags.IBooleanFlag<any> }
private readonly context: any
private _currentFlag: Flags.IOptionFlag<any> | null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use an underscore here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, the convention in oclif code is to use undefined instead of null which avoids the defaulting you do below

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in other words, this should be private currentFlag?: Flags.IOptionFlag<any>

src/parse.ts Outdated
@@ -47,8 +47,10 @@ export class Parser<T extends ParserInput, TFlags extends OutputFlags<T['flags']
private readonly raw: ParsingToken[] = []
private readonly booleanFlags: { [k: string]: Flags.IBooleanFlag<any> }
private readonly context: any
private _currentFlag: Flags.IOptionFlag<any> | null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, the convention in oclif code is to use undefined instead of null which avoids the defaulting you do below

})

it('flag multiple with arguments', () => {
const out = parse(['--foo', './a.txt', './b.txt', './c.txt', '--', '15'], {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice job catching this case, I wasn't sure how it would behave

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

than you :)

@jdx
Copy link
Contributor

jdx commented May 31, 2018

I enabled circle to run on forked builds so it should build on the next push. I'm looking into why appveyor is failing, but that is unrelated to this PR. You submitted your last push just before I put a note in about using null (not sure if you saw that).

If you fix the null thing it should trigger a circle build and we can merge this

@codecov
Copy link

codecov bot commented Jun 1, 2018

Codecov Report

Merging #25 into master will increase coverage by 0.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
+ Coverage   90.26%   90.39%   +0.12%     
==========================================
  Files          11       11              
  Lines         298      302       +4     
  Branches       79       80       +1     
==========================================
+ Hits          269      273       +4     
  Misses         10       10              
  Partials       19       19
Impacted Files Coverage Δ
src/parse.ts 91.24% <100%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2747c72...2b3c094. Read the comment docs.

Copy link
Contributor

@jdx jdx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice job! I'm actually surprised you were able to do this. My first reaction is that this wouldn't (easily) work with other flags or -- but it seems to work great!

@jdx jdx merged commit c636d74 into oclif:master Jun 1, 2018
oclif-bot added a commit that referenced this pull request Jun 1, 2018
<a name="3.5.0"></a>
# [3.5.0](d1b77f1...v3.5.0) (2018-06-01)

### Features

* Handle flags with multiple inputs without the flag before each input ([#25](#25)) ([c636d74](c636d74))
@idandrd
Copy link
Contributor Author

idandrd commented Jun 1, 2018

Thank you very much!
BTW, thanks a lot for creating Oclif, it really helps a lot in our startup.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants