Global vs. local options

[nowsprinting]

When I use the -c (--chdir) option I get the following error:

error: unknown option '-c'

It was working before, and the README still mentions it.

[ComradeVanti]

You are right. Actually a recent change I made broke all global options. They now need to be appended after the command. Ie openupm add -c some/path com.some.package. But this was a mistake. I will revert it but in the meantime that is how you can get around it.

[ComradeVanti]

Actually now that I think about it, this is something that I would like to put up for discussion a bit. So just to be clear:

Previously there were global options which could be applied to the openupm base-command. They could always be put there even if they did not make sense for the used subcommand. For example you could specify the -c flag for the view command even though it is not used there.

Now each subcommand has it's own options and only uses those that it actually needs. For example add has the -c option, but view does not. This makes more sense but also means that current users might be confused. This new behavior would also need to be described in the documentation.

@favoyang what is your take on this? Also @nowsprinting you are welcome to give your input :)

[ComradeVanti]

Also there are still global options, such as --no-color which are applied directly to openupm as a whole.

[favoyang]

Now each subcommand has it's own options and only uses those that it actually needs. For example add has the -c option, but view does not. This makes more sense but also means that current users might be confused. This new behavior would also need to be described in the documentation.

@favoyang what is your take on this?

I support leaving -c as a subcommand parameter for add and remove.

[ComradeVanti]

I test rewrote the docs for the add command to reflect the change at #409. Feedback welcome.

[favoyang]

LGTM, though I would like to keep a direct link for help-registry.md on the doc homepage.

[nowsprinting]

Thanks!

I think -c option should be global. The reason is follows:

1. It is valid option for the add and remove subcommands.
2. The main feature of openupm-cli is add/remove package to project, so there is not discomfort in specifying the project as global.

[ComradeVanti]

I personally side with @favoyang to have the option be added to the specific command. My reasoning is:

1. Just technical correctness. The option does nothing for the view command, but it does for the add command. Therefore it should only be added to the command that uses it.
2. Having options just be on the command that use them is the more common thing in other cli programs also I think.

The main feature of openupm-cli is add/remove package to project, so there is not discomfort in specifying the project as global.

While that is true, I think this reasoning is off. For example when you use git you often commit, that is one of the main commands you run. There you have the -m option, for specifying the commit message. I dont think it makes sense to make -m part of the global git command. Users might be confused why they can enter a message for git bisect or whatever else. Similarly, users might be confused why they can change the working directory for openupm deps. "Will this have any effect on the command? Do I have to run it in a Unity project?". I know this is an issue because I actually had this exact question myself while developing. In the code I saw that the working directory option is parsed as part of the deps command and was confused about why.

So yes in total I think I should keep it the way it is now but with two corrections:

1. Good docs to describe how to use the commands now.
2. Maybe a global dummy option -c which tells the user that the option has moved.

[nowsprinting]

It's good to organize the specifications.
But it's an intentional breaking change. I expected to bump the major version.

[ComradeVanti]

But it's an intentional breaking change. I expected to bump the major version.

Very fair. I will do that soon.

[ComradeVanti]

I opened a PR incorporating the deprecation warnings #410. I'll close this discussion now