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

Clarity for commands vs global options #8

Merged
merged 1 commit into from
Dec 11, 2015
Merged

Clarity for commands vs global options #8

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions runtime.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@ $ funC [global-options] <COMMAND> [command-specific-options] <command-specific-a

## Global options

None are required, but the runtime may support options that start with at least one hyphen.
Global options may take positional arguments (e.g. `--log-level debug`), but the option parsing must be such that `funC <COMMAND>` is unambiguously an invocation of `<COMMAND>` for any `<COMMAND>` that does not start with a hyphen (including commands not specified in this document).
None are required, but the runtime MAY support options that start with at least one hyphen.
Global options MAY take positional arguments (e.g. `--log-level debug`).
Command names MUST not start with hyphens.
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure about this MUST. It's a requirement we're imposing on this specification so runtime authors know what to expect, and not a requirement that we're imposing on the runtime. On the other hand, I think folks will generally get the idea even if we don't get this quite right ;). Do you have a feeling for which way would be right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this works. You're not saying someone can't do something like a command with a hyphenated global option. You're say, instead, that the spec has certain commands that are not hyphenated and they follow the global options.

Copy link
Owner

Choose a reason for hiding this comment

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

On Mon, Dec 07, 2015 at 03:34:01PM -0800, Mike Brown wrote:

+Command names MUST not start with hyphens.

… You're say, instead, that the spec has certain commands that are
not hyphenated and they follow the global options.

Right, so what's MUST about it? There's no way runtime implementors
could get that wrong, since it's a requirement we're making of the
spec itself. See also my suggested test-case criterion 1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're saying '-START' is not the same as 'START' wrt compliance. Rather -START path in front of the command START would be a global option. You're also limiting the spec writers from muddying commands and global options. You're forcing them to have a command on every invoke.

Copy link
Owner

Choose a reason for hiding this comment

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

On Mon, Dec 07, 2015 at 03:54:55PM -0800, Mike Brown wrote:

+Command names MUST not start with hyphens.

You're saying -START is not the same as START wrt compliance.

No, I'm saying:

The only commands we define now are ‘version’, ‘start’, ‘exec’,
‘pause’, ‘resume’, and ‘signal’. We may release a future version of
this spec with a command like ‘checkpoint’, but will not release any
future versions of this spec with a command like ‘-checkpoint’.

Without a pattern like “commands will not start with a hyphen” it
would be harder to distinguish unrecognized commands from unrecognized
options, and we're requiring runtimes to fail-fast for unrecognized
commands.

I agree that ‘-start’ and ‘start’ are not the same, but I think that's
clear without this sentence.

Copy link
Owner

Choose a reason for hiding this comment

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

On Mon, Dec 07, 2015 at 04:52:47PM -0800, Mike Brown wrote:

Your statements clarified for me how you read them, specifically
with the future guarantees and the purpose of making easier to
parse. I don't think we need to add any statements about future
revisions or the rationale for the requirement. It's the pattern
selected .. so it is what it is.

Works for me. Even if we don't change the spec text, I think it's
worth being very explicit about the rationale and our intentions in
the commit message. That way we don't have to rehash the reasoning if
we do decide that the spec needs clarification later. Can you update
the commit message with something to cover the points made in [1,2]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point done :-)

Copy link
Owner

Choose a reason for hiding this comment

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

On Mon, Dec 07, 2015 at 09:30:38PM -0800, Mike Brown wrote:

+Command names MUST not start with hyphens.

Good point done :-)

But not force-pushed? I'm still seeing d67df10 anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I just put it in the PR comment. I refreshed the commit. Not used to putting long winded commit messages in. Cheers.

Copy link
Owner

Choose a reason for hiding this comment

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

On Mon, Dec 07, 2015 at 10:13:31PM -0800, Mike Brown wrote:

Yeah I just put it in the PR comment. I refreshed the commit. Not
used to putting long winded commit messages in.

2b084a3 LGTM. I'm fine with it landing as is, but here are a few
notes on the commit message:

Discussed and worked on wording with @wking via private IRC.

This^ isn't important enough to include in the commit message. Folks
reading this will care about what changed, what our motivation was,
and why we implemented it the way we did. They don't care who we were
(except for copyright ownership, and Git's Author: covers that) or
what channel we used for the discussion. I'll my Reviewed-by when
this lands, so folks will know that I signed-off on it (and we'll add
Reviewed-by tags for anyone else that LGTMs it too).

"distinguish unrecognized commands…

This^ is missing a closing quote.

[1] https…

No need to escape these^ references (e.g. see 3606bcf). Git's command
line tools don't markup the message, and GitHub links URLs but doesn't
render Markdown in the messages.

For motivating long-winded commit messages, see [1,2]. I like them
because you can go straight from ‘git blame’ to notes motivating that
line, instead of going blame → commit hash → GitHub PR → some comment
in that discussion. And I have a local mirror of the commits (thanks
Git :), but I don't keep a local mirror of the GitHub PR discussion.

The option parsing MUST be such that `funC <COMMAND>` is unambiguously an invocation of `<COMMAND>` (even for commands not specified in this document).
If the runtime is invoked with an unrecognized command, it MUST exit with a nonzero exit code and MAY log a warning to stderr.

## Character encodings

Expand Down