pkg/args Option should be more strict about names and abbreviations

pkg/args Option should be more strict about names and abbreviations

Should not allow: space, \t, \r, \n, single- and double-quote, slashes
Should not allow empty string
Should not allow '-' as the beginning character

name should not be allowed to be null (although it's fine for abbreviation)

BUG=https://code.google.com/p/dart/issues/detail?id=8965

Committed: https://code.google.com/p/dart/source/detail?r=20040

[kevmoo-old, 2013-03-06 23:59:29]

PTAL

I went for the obvious ones that would cause most issues, especially slashes,
whitespace, and leading dashes

If this looks good, the logic should probably be applied to command names as
well.

[Bob Nystrom, 2013-03-07 00:11:29]

On 2013/03/06 23:59:29, kevmoo wrote:
> PTAL
> 
> I went for the obvious ones that would cause most issues, especially slashes,
> whitespace, and leading dashes
> 
> If this looks good, the logic should probably be applied to command names as
> well.

I'm going on vacation for a few days, but I'll take a look at this next week
when I get back.

[Bob Nystrom, 2013-03-13 18:07:09]

Thanks for doing this!

https://codereview.chromium.org/12472019/diff/1/pkg/args/lib/args.dart
File pkg/args/lib/args.dart (right):

https://codereview.chromium.org/12472019/diff/1/pkg/args/lib/args.dart#newcod...
pkg/args/lib/args.dart:352: if(name == null) {
Space between keyword and "(" here and elsewhere ("if", "for", "while", etc.).

https://codereview.chromium.org/12472019/diff/1/pkg/args/lib/args.dart#newcod...
pkg/args/lib/args.dart:353: throw new ArgumentError('"name" cannot be null');
As dumb as it seems, almost all Dart code I've seen doesn't bother to explicitly
check for null arguments, so let's remove this.

https://codereview.chromium.org/12472019/diff/1/pkg/args/lib/args.dart#newcod...
pkg/args/lib/args.dart:355: throw new ArgumentError('"name" cannot be empty');
The quoting here is confusing. How about "Name cannot be empty.".

Can you make these error messages complete sentences too?

https://codereview.chromium.org/12472019/diff/1/pkg/args/lib/args.dart#newcod...
pkg/args/lib/args.dart:357: throw new ArgumentError('"name" cannot start with
"-"');
Would be nice to show the offending argument: 'Name "$name" cannot start with
"-".'

https://codereview.chromium.org/12472019/diff/1/pkg/args/lib/args.dart#newcod...
pkg/args/lib/args.dart:360: // ensure name does not contain any invalid
characters
Sentence capitalize and add a ".".

https://codereview.chromium.org/12472019/diff/1/pkg/args/lib/args.dart#newcod...
pkg/args/lib/args.dart:361: for(final char in _invalidChars) {
This seems like a good case for a regexp:

if (_invalidChars.hasMatch(name)) ...

where _invalidChars is:

final _invalidChars = new RegExp("[ \t\r\n\"'\\/]");

https://codereview.chromium.org/12472019/diff/1/pkg/args/lib/args.dart#newcod...
pkg/args/lib/args.dart:379: throw new ArgumentError('"abbreviation" is an
invalid valid');
This error message is a little odd. :)

https://codereview.chromium.org/12472019/diff/1/pkg/args/lib/args.dart#newcod...
pkg/args/lib/args.dart:385: const _invalidChars = const [' ', '\t', '\r', '\n',
'"', '"', r'\', r'/'];
Since it isn't instance-specific and is private, how about we hoist this out of
the class?

https://codereview.chromium.org/12472019/diff/1/pkg/args/test/args_test.dart
File pkg/args/test/args_test.dart (right):

https://codereview.chromium.org/12472019/diff/1/pkg/args/test/args_test.dart#...
pkg/args/test/args_test.dart:39: for(final name in _invalidOptions) {
To be consistent, we don't use final for local variables.

https://codereview.chromium.org/12472019/diff/1/pkg/args/test/args_test.dart#...
pkg/args/test/args_test.dart:224: Iterable<String> get _invalidOptions {
This may be a big gratuitous. :)

How about just a fixed list of strings with one containing each black-listed
character and some other characters? Also an empty string one too.

[kevmoo-old, 2013-03-13 19:30:48]

Comments on comments.

Updated CL in progress.

https://codereview.chromium.org/12472019/diff/1/pkg/args/test/args_test.dart
File pkg/args/test/args_test.dart (right):

https://codereview.chromium.org/12472019/diff/1/pkg/args/test/args_test.dart#...
pkg/args/test/args_test.dart:39: for(final name in _invalidOptions) {
On 2013/03/13 18:07:09, Bob Nystrom wrote:
> To be consistent, we don't use final for local variables.

`final` is so awesome. I'd love someone to start a trend to the contrary.

...but I won't be that guy.

https://codereview.chromium.org/12472019/diff/1/pkg/args/test/args_test.dart#...
pkg/args/test/args_test.dart:224: Iterable<String> get _invalidOptions {
On 2013/03/13 18:07:09, Bob Nystrom wrote:
> This may be a big gratuitous. :)

I take testing seriously.

[kevmoo-old, 2013-03-13 19:35:53]

Bob: thoughts?

https://codereview.chromium.org/12472019/diff/1/pkg/args/lib/args.dart
File pkg/args/lib/args.dart (right):

https://codereview.chromium.org/12472019/diff/1/pkg/args/lib/args.dart#newcod...
pkg/args/lib/args.dart:385: const _invalidChars = const [' ', '\t', '\r', '\n',
'"', '"', r'\', r'/'];
On 2013/03/13 18:07:09, Bob Nystrom wrote:
> Since it isn't instance-specific and is private, how about we hoist this out
of
> the class?

My theory: since it's *only* used by the class, we keep the top-level namespace
clean and keep the definition close to the usage.

...I'm happy to defer to guidelines and convention, but I'd like to understand.

[kevmoo-old, 2013-03-13 19:46:17]

Updated for Bob's notes.

PTAL and see my comments.

https://codereview.chromium.org/12472019/diff/6002/pkg/args/lib/args.dart
File pkg/args/lib/args.dart (right):

https://codereview.chromium.org/12472019/diff/6002/pkg/args/lib/args.dart#new...
pkg/args/lib/args.dart:353: throw new ArgumentError('Name cannot be null.');
Seemed too weird to test for a Method Not Found error.

Aside from the fact that this is much more helpful for an end user.

Not trying to buck tradition...just nudge it a bit.

https://codereview.chromium.org/12472019/diff/6002/pkg/args/lib/args.dart#new...
pkg/args/lib/args.dart:378: final _invalidChars = new RegExp(r'''[
\t\r\n"'\\/]''');
this is only used by the ctor of this class. I'm a bit fan of

1) keeping definitions close to their usage
2) minimizing top-level mess.

Again, not trying to buck tradition...just nudge it.

[Bob Nystrom, 2013-03-13 21:29:57]

As far as top-level versus in a class (for private stuff), my thoughts are:

1. Keep your libraries small and it doesn't matter much one way or the other.
2. If it doesn't access any instance state on the class, it should absolutely be
static if not top-level.
3. Top-level stuff is a bit more terse.
4. Top-level stuff makes it abundantly clear that the function doesn't access
any instance state on the class.
5. I think it's important for people to internalize library privacy, so I like
using top-level private stuff to reinforce that mental model.

But I don't have a very strong preference here. It just irks me when I see Dart
code look like verbose Java code. :)

https://codereview.chromium.org/12472019/diff/1/pkg/args/test/args_test.dart
File pkg/args/test/args_test.dart (right):

https://codereview.chromium.org/12472019/diff/1/pkg/args/test/args_test.dart#...
pkg/args/test/args_test.dart:39: for(final name in _invalidOptions) {
On 2013/03/13 19:30:48, kevmoo wrote:
> On 2013/03/13 18:07:09, Bob Nystrom wrote:
> > To be consistent, we don't use final for local variables.
> 
> `final` is so awesome. I'd love someone to start a trend to the contrary.
> 
> ...but I won't be that guy.

If we had "val" I would be all about it. In the absence of that "var" is shorter
and more familiar. Given that we tend to keep methods pretty short I'm honestly
not convinced enforcing single-assignment for local variables really adds much.
(Fields are another matter of course.)

https://codereview.chromium.org/12472019/diff/1/pkg/args/test/args_test.dart#...
pkg/args/test/args_test.dart:224: Iterable<String> get _invalidOptions {
On 2013/03/13 19:30:48, kevmoo wrote:
> On 2013/03/13 18:07:09, Bob Nystrom wrote:
> > This may be a big gratuitous. :)
> 
> I take testing seriously.

Testing is serious business. SGTM.

https://codereview.chromium.org/12472019/diff/6002/pkg/args/lib/args.dart
File pkg/args/lib/args.dart (right):

https://codereview.chromium.org/12472019/diff/6002/pkg/args/lib/args.dart#new...
pkg/args/lib/args.dart:353: throw new ArgumentError('Name cannot be null.');
On 2013/03/13 19:46:17, kevmoo wrote:
> Seemed too weird to test for a Method Not Found error.

Yeah, don't test for that either.

> Aside from the fact that this is much more helpful for an end user.
> 
> Not trying to buck tradition...just nudge it a bit.

I'm not crazy about the status quo here, but I think it's valuable to be
consistent and keep code simple. Most public APIs do not explicitly check for
null, and most tests of those APIs don't validate what happens when you pass
null.

In other words, almost everyone on the team acts as if types are not nullable,
even though they are.

https://codereview.chromium.org/12472019/diff/6002/pkg/args/lib/args.dart#new...
pkg/args/lib/args.dart:378: final _invalidChars = new RegExp(r'''[
\t\r\n"'\\/]''');
On 2013/03/13 19:46:17, kevmoo wrote:
> this is only used by the ctor of this class.

Fair point.

> 1) keeping definitions close to their usage
> 2) minimizing top-level mess.

Top level stuff doesn't bother me if it's private. I like small libraries, so
the private top-level namespace is small and uncluttered.

Leaving it in the class is fine if you prefer it. If you do, it should be
static.

[kevmoo-old, 2013-03-13 21:41:39]

More tweaks per notes.

PTAL

https://codereview.chromium.org/12472019/diff/12001/pkg/args/lib/args.dart
File pkg/args/lib/args.dart (right):

https://codereview.chromium.org/12472019/diff/12001/pkg/args/lib/args.dart#ne...
pkg/args/lib/args.dart:353: if (name.isEmpty) {
Removed the null check. And the test for null.

Grinning through my teeth. :-)

https://codereview.chromium.org/12472019/diff/12001/pkg/args/lib/args.dart#ne...
pkg/args/lib/args.dart:377: static final _invalidChars = new RegExp(r'''[
\t\r\n"'\\/]''');
*Totally* spaced on the static bit. No argument there.

[Bob Nystrom, 2013-03-14 17:37:11]

One style nit, then LGTM!

https://codereview.chromium.org/12472019/diff/15001/pkg/args/test/args_test.dart
File pkg/args/test/args_test.dart (right):

https://codereview.chromium.org/12472019/diff/15001/pkg/args/test/args_test.d...
pkg/args/test/args_test.dart:225: [
Put the "[" on the preceding line, and the "]" on its own, like:

const _invalidOptions = const [
  ...
];

Also, since it's const, make the name ALL_CAPS (or just don't use const).

[kevmoo-old, 2013-03-14 18:00:28]

Committed patchset #5 manually as r20040 (presubmit successful).