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.

Sign up for GitHub

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

Jump to bottom

Added consistency in commands for project, application & components #421

Merged
merged 1 commit into from
May 23, 2018
Merged

Added consistency in commands for project, application & components #421

merged 1 commit into from
May 23, 2018

Conversation

surajnarwade
Copy link
Contributor

Fixes #274

Now,

odo project xyz

odo component abc

odo application pqr

they will work same as set operation.

@surajnarwade surajnarwade requested review from mik-dass and concaf April 26, 2018 09:23
@kadel
Copy link
Member

kadel commented Apr 26, 2018

What if I create a project called list? 😇

@mik-dass
Copy link
Contributor

@surajnarwade @kadel is right, it breaks on define keywords like list, create and others

New project created and now using project : create
[mdas@localhost odo]$ ./odo project create
Error: accepts 1 arg(s), received 0
Usage:
  odo project create [flags]

Flags:
  -h, --help   help for create

Global Flags:
  -v, --verbose   Verbose output

accepts 1 arg(s), received 0```

@kadel
Copy link
Member

kadel commented Apr 26, 2018

The question is how much do we care ;-)

@mik-dass
Copy link
Contributor

I think we should just leave it with set to switch for now

@surajnarwade
Copy link
Contributor Author

@kadel , I think we should not care about this corner case, since list is verb ?
/me still confused though

@surajnarwade
Copy link
Contributor Author

cc @kadel @mik-dass

@kadel
Copy link
Member

kadel commented May 3, 2018

Looks good. But we need to add examples and descriptions. Otherwise, users won't know what is going on.

@surajnarwade
Copy link
Contributor Author

@kadel @mik-dass another round of review needed, added example as per suggestion

kadel
kadel reviewed May 11, 2018
View reviewed changes
cmd/application.go Outdated
Run: applicationGetCmd.Run,
Example: ` # Get current application,
odo app
# Set webapp to current application,
Copy link
Member

Choose a reason for hiding this comment

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

can you please add an empty line between each example?
It would be nice to keep it consistent with other places like odo create -h

cmd/component.go Outdated
)

// componentCmd represents the component command
var componentCmd = &cobra.Command{
Use: "component",
Short: "Components of application.",
Example: ` # Get current component,
odo component
# Set nodejs to current component,
Copy link
Member

Choose a reason for hiding this comment

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

can you please add an empty line between each example?
It would be nice to keep it consistent with other places like odo create -h

cmd/project.go Outdated
@@ -3,9 +3,10 @@ package cmd
import (
"fmt"

Copy link
Member

Choose a reason for hiding this comment

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

empty line

cmd/project.go Outdated
Run: projectGetCmd.Run,
Example: ` # Get current project,
odo project
# Set myproject to current project,
Copy link
Member

Choose a reason for hiding this comment

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

can you please add an empty line between each example?
It would be nice to keep it consistent with other places like odo create -h

@surajnarwade surajnarwade force-pushed the consistency branch 4 times, most recently from a4d452c to a39922b Compare May 15, 2018 09:30
@surajnarwade
Copy link
Contributor Author

@kadel done with suggested changes

@kadel kadel added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label May 18, 2018
@kadel
Copy link
Member

kadel commented May 22, 2018

good to go after rebase

@surajnarwade
Added consistency in commands for project, application & components
7a5ae72 
Fixes redhat-developer#274

Now,

```
odo project xyz

odo component abc

odo application pqr
```

they will work same as `set` operation.
@surajnarwade surajnarwade removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label May 23, 2018
@kadel kadel merged commit af29488 into redhat-developer:master May 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kadel kadel kadel approved these changes

@mik-dass mik-dass Awaiting requested review from mik-dass

@concaf concaf Awaiting requested review from concaf

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@surajnarwade @kadel @mik-dass