Skip to content
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

Drop interactive mode #12

Merged
merged 4 commits into from
Jun 28, 2017
Merged

Drop interactive mode #12

merged 4 commits into from
Jun 28, 2017

Conversation

Tom32i
Copy link
Contributor

@Tom32i Tom32i commented Jun 17, 2017

  • Gad don't stay "open" after executing one command
  • Commands now supports arguments (e.g. gad changelog --all to see all issues for the sprint)
  • Errors are properly caught

@Tom32i Tom32i changed the title [WIP] Drop interactive mode Drop interactive mode Jun 27, 2017
@Tom32i Tom32i requested review from chalasr and ogizanagi June 27, 2017 12:10
Copy link
Contributor

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

👍 Nice.
About using && for running several commands, won't this conflict with the native unix &&? For instance, running gad sprint && review && vagrant up won't work anymore, right?

@Tom32i
Copy link
Contributor Author

Tom32i commented Jun 27, 2017

@chalasr It's slightly better than that: the && will still be parsed by your terminal by default:

  • So gad sprint && review && vagrant up would be interpreted as gad sprint + review + vagrant up (and will probably fail because of `review).
  • But gad "sprint && review" && vagrant up will work as expected.

I you think this do more wrong than right, I'm prepared to remove it.

Copy link
Member

@ogizanagi ogizanagi left a comment

Choose a reason for hiding this comment

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

I'm not sure either about the several commands features. IMHO we should simply leverage on basic shell features for such things.

Copy link
Contributor

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Yea I would remove it, that will prevent from wtf moments

@chalasr
Copy link
Contributor

chalasr commented Jun 27, 2017

Just played a bit with this, the following runs fine:
gad -o [org] -r [repo] [command] --[command-options]
but the following doesn't:
gad [command] -o [org] -r [repo] --[command-options]
it leads to:

⏳ Fetching issues...
{ [Error: Empty value for parameter 'owner': null]
defaultMessage: 'Bad Request',
message: 'Empty value for parameter 'owner': null',
code: '400',
status: 'Bad Request',
headers: undefined }

Firstly, I think we should add some validation rules to the options and validate them before performing any http request or writing anything to the output, in order to provide meaningful error messages when an option is missing or has an invalid value.
The first thing I would validate, now that you removed the interactive mode, is that a valid command has been passed.
Actually running just gad within this PR leads to the very same error (Bad Request, null owner). I would love to get an "help" displayed in this case (list of all commands with options, same as the README table)
Running gad foobar leads to the same. I would love to get something like Invalid command "foobar". [help] in this case. WDYT?

Secondly, is it expected that global options (e.g. --owner, --repo) must be passed before the command name and options?
I think it would be nice if my 2nd (failing) try had worked as the first one. Is it doable?
That means kind of merging global options with command ones so that neither their order nor their position matters, which makes sense I think.

@Tom32i Tom32i mentioned this pull request Jun 28, 2017
@Tom32i
Copy link
Contributor Author

Tom32i commented Jun 28, 2017

@chalasr I moved your concerns in a specific issue.

@chalasr
Copy link
Contributor

chalasr commented Jun 28, 2017

👌 Merging then

@chalasr chalasr merged commit a48061c into master Jun 28, 2017
@chalasr chalasr deleted the drop-interactive-mode branch June 28, 2017 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants