Arvados

Reviewing d827740

• In the refactoring, the semantics of arv pipeline have been changed. It's a subcommand that has its own subcommands, similar to arv keep, although right now it only accepts one (run). I think that behavior needs to be restored.
• It would be nicely user-friendly if arv edit reported parse errors in the edited file and asked the user whether they wanted to retry the edit or abort. visudo does this, and it helps smooth over the typo-fixing process.
• If the editor gives a nonzero exit code, I think arv edit should report a brief message about that and exit nonzero as well. This would match the behavior of other tools like git when they spawn editors.
• Following the discussion we had with Ward a little while back, I think we should always prefer Oj to JSON from now on. Part of arv edit uses JSON.parse.
• The --resources option has been removed when we generate the parser, which seems safe enough since it's basically just another --help, but other code still checks whether it's set. It'd be nice to clean those up.
• The help message for arv edit does not use a consistent verb tense. The first sentence is a run-on sentence. There's a typo in "fetchs."
• In other messages throughout, I think "Arvados" should always be capitalized.
• If I try to arv edit a Collection, it will tell me that the object isn't an Arvados object. That may be strictly true, but I worry it could confuse newer users who don't understand the distinction.

Thanks.

Brett Smith wrote:

Reviewing d827740

• In the refactoring, the semantics of arv pipeline have been changed. It's a subcommand that has its own subcommands, similar to arv keep, although right now it only accepts one (run). I think that behavior needs to be restored.

Fixed.

• It would be nicely user-friendly if arv edit reported parse errors in the edited file and asked the user whether they wanted to retry the edit or abort. visudo does this, and it helps smooth over the typo-fixing process.

Fixed, after I got bit by this before you even suggested it.

• If the editor gives a nonzero exit code, I think arv edit should report a brief message about that and exit nonzero as well. This would match the behavior of other tools like git when they spawn editors.

Fixed.

• Following the discussion we had with Ward a little while back, I think we should always prefer Oj to JSON from now on. Part of arv edit uses JSON.parse.

I actually thought we were supposed to be using multi-json or something. The plethora of json parsers for Ruby makes my head hurt.

• The --resources option has been removed when we generate the parser, which seems safe enough since it's basically just another --help, but other code still checks whether it's set. It'd be nice to clean those up.

What "other code" are you referring to?

• The help message for arv edit does not use a consistent verb tense. The first sentence is a run-on sentence. There's a typo in "fetchs."

Fixed.

• In other messages throughout, I think "Arvados" should always be capitalized.

I only found one place that was the case, other strings containing "arvados" were not output strings.

• If I try to arv edit a Collection, it will tell me that the object isn't an Arvados object. That may be strictly true, but I worry it could confuse newer users who don't understand the distinction.

Eh, collections aren't supposed to be editable anyway.

Peter Amstutz wrote:

Brett Smith wrote:

• The --resources option has been removed when we generate the parser, which seems safe enough since it's basically just another --help, but other code still checks whether it's set. It'd be nice to clean those up.

What "other code" are you referring to?

if global_opts[:resources] inside parse_arguments.

• If I try to arv edit a Collection, it will tell me that the object isn't an Arvados object. That may be strictly true, but I worry it could confuse newer users who don't understand the distinction.

Eh, collections aren't supposed to be editable anyway.

Agreed. The most I was asking for is a message that specifically says that, rather than claiming it's not an Arvados object.

1. Fixed
2. Fixed

b653edb

b653edb looks good to merge to me. Thanks.