Issue 11938028: Experimental analyzer front-end baby-steps.

Issue 11938028: Experimental analyzer front-end baby-steps. (Closed)

Created:
7 years, 11 months ago by pquitslund

Modified:
7 years, 11 months ago

Reviewers:
Bob Nystrom, scheglov, danrubel, Brian Wilkerson

CC:
kasperl

Base URL:
http://dart.googlecode.com/svn/branches/bleeding_edge/dart/

Visibility:
Public.

More Reviews

Description

Experimental analyzer front-end baby-steps. Notably: * basic package layout, * commandline parsing, and * a few tests Much more to come! :) Committed: https://code.google.com/p/dart/source/detail?r=17414

Patch Set 1 #

Total comments: 3

Patch Set 2 : #

Total comments: 26

Created: 7 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+297 lines, -0 lines)			Patch
A	pkg/analyzer-experimental/bin/analyzer.dart	View		1 chunk	+46 lines, -0 lines	6 comments	Download
A	pkg/analyzer-experimental/lib/analyzer.dart	View		1 chunk	+4 lines, -0 lines	0 comments	Download
A	pkg/analyzer-experimental/lib/options.dart	View	1	1 chunk	+196 lines, -0 lines	20 comments	Download
A	pkg/analyzer-experimental/pubspec.yaml	View		1 chunk	+8 lines, -0 lines	0 comments	Download
A	pkg/analyzer-experimental/test/options_test.dart	View		1 chunk	+43 lines, -0 lines	0 comments	Download

Messages

Total messages: 11 (0 generated)

Expand Messages | Collapse Messages

pquitslund

https://codereview.chromium.org/11938028/diff/1/pkg/analyzer-experimental/lib/analyzer.dart File pkg/analyzer-experimental/lib/analyzer.dart (right): https://codereview.chromium.org/11938028/diff/1/pkg/analyzer-experimental/lib/analyzer.dart#newcode4 pkg/analyzer-experimental/lib/analyzer.dart:4: Just a place-holder for now.

7 years, 11 months ago (2013-01-18 20:56:04 UTC) #1

Brian Wilkerson

LGTM https://codereview.chromium.org/11938028/diff/1/pkg/analyzer-experimental/lib/options.dart File pkg/analyzer-experimental/lib/options.dart (right): https://codereview.chromium.org/11938028/diff/1/pkg/analyzer-experimental/lib/options.dart#newcode96 pkg/analyzer-experimental/lib/options.dart:96: '<libraries to analyze...]'); Was the '<' suppose to ...

7 years, 11 months ago (2013-01-18 21:14:33 UTC) #4

pquitslund

https://codereview.chromium.org/11938028/diff/1/pkg/analyzer-experimental/lib/options.dart File pkg/analyzer-experimental/lib/options.dart (right): https://codereview.chromium.org/11938028/diff/1/pkg/analyzer-experimental/lib/options.dart#newcode96 pkg/analyzer-experimental/lib/options.dart:96: '<libraries to analyze...]'); On 2013/01/18 21:14:33, Brian Wilkerson wrote: ...

7 years, 11 months ago (2013-01-18 21:22:19 UTC) #6

Bob Nystrom

Random feedback to do with as you will. https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/bin/analyzer.dart File pkg/analyzer-experimental/bin/analyzer.dart (right): https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/bin/analyzer.dart#newcode7 pkg/analyzer-experimental/bin/analyzer.dart:7: /** ...

7 years, 11 months ago (2013-01-18 22:25:21 UTC) #8

pquitslund

Awesome feedback. Thanks! You just might make a darter out of me yet! :) https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/bin/analyzer.dart ...

7 years, 11 months ago (2013-01-22 18:31:53 UTC) #9

Awesome feedback.  Thanks!  You just might make a darter out of me yet! :)

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/...
File pkg/analyzer-experimental/bin/analyzer.dart (right):

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/...
pkg/analyzer-experimental/bin/analyzer.dart:7: /** The entry point for the
analyzer */
On 2013/01/18 22:25:22, Bob Nystrom wrote:
> Make full sentence: "."

Done.

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/...
pkg/analyzer-experimental/bin/analyzer.dart:15: //Exit status codes
On 2013/01/18 22:25:22, Bob Nystrom wrote:
> Space after "//" and "." at the end.

Done.

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/...
pkg/analyzer-experimental/bin/analyzer.dart:41: return new Future.immediate(new
AnalysisResult.forFailure());
On 2013/01/18 22:25:22, Bob Nystrom wrote:
> I would probably separate *analysis* failure from argument parsing failure.
You
> can throw a FormatException from this method and then "catch" it in main()
using
> catchError() on Future, like:
> 
>    run(new Options().arguments).then((result) {
>      ...
>    }).catchError((error) {
>      if (error.error is FormatException) {
>        ...
>      }
>    });

Since I've already handled a FormatException in parse() (in order to print
usage), throwing another feels odd here unless, that is, I want different exit
codes for analysis and format errors, which I just might.  Anyway, I'm on the
fence enough that I'll probably sit on this for now.   But thanks!

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/...
File pkg/analyzer-experimental/lib/options.dart (right):

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/...
pkg/analyzer-experimental/lib/options.dart:20: static final String
_DEFAULT_SDK_LOCATION = Platform.environment[_SDK_ENV];
On 2013/01/18 22:25:22, Bob Nystrom wrote:
> It's OK either way, but I tend to hoist private constants out of the class and
> into the top level. Maybe I just don't like writing "static const" and "static
> final". :)

Interesting... I'm still feeling my way around this top-level namespace, 
Anyway, advice taken!

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/...
pkg/analyzer-experimental/lib/options.dart:59: static CommandLineOptions
parse(List<String> args) {
On 2013/01/18 22:25:22, Bob Nystrom wrote:
> Make this a factory constructor and make .fromArgs() private? Then you could
do:
> 
> new CommandLineOptions.parse(args);

Nice.  Thanks.

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/...
pkg/analyzer-experimental/lib/options.dart:106: * options/flags, this class can
be replaced with a simple [ArgParser] instance.
On 2013/01/18 22:25:22, Bob Nystrom wrote:
> Mention #5262 here.

On the surface, that doesn't seem to capture this specific issue.  Or?  If you
agree, I'll open another ticket and point this to it.

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/...
pkg/analyzer-experimental/lib/options.dart:111: final ArgParser _parser;
On 2013/01/18 22:25:22, Bob Nystrom wrote:
> Is there a reason you're forwarding instead of inheriting and overriding?

Habit.  I generally avoid subclassing where it doesn't have clear benefits. 
Blame my caution on the indoctrination of the Go4!

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/...
pkg/analyzer-experimental/lib/options.dart:127: _parser.addFlag(name, abbr:abbr,
help:help, defaultsTo:defaultsTo,
On 2013/01/18 22:25:22, Bob Nystrom wrote:
> Space after ":" here and elsewhere.

My kingdom for a formatter! :)

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/...
pkg/analyzer-experimental/lib/options.dart:162: //Only filter if the ignore flag
is specified
On 2013/01/18 22:25:22, Bob Nystrom wrote:
> Space after "//" and make a sentence.

Done.

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/...
pkg/analyzer-experimental/lib/options.dart:167: //Filter all unrecognized flags
and options
On 2013/01/18 22:25:22, Bob Nystrom wrote:
> Ditto.

Done.

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/...
pkg/analyzer-experimental/lib/options.dart:168: var filtered = <String>[];
On 2013/01/18 22:25:22, Bob Nystrom wrote:
> return args.where((arg) => !arg.startsWith('--') ||
>                            _knownFlags.contains(arg.substring(2));
> 
> :)

Cheeky! :)  But I think this is missing the parameter eating part.  That is, I
think this should work for options as well.  Or?  IOW, seeing this

 --ignore me --but not any of me 

should yield ['--but', 'not', 'any', 'of', 'me'] assuming 'but' is an understood
option.

What do you think?

Bob Nystrom

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/lib/options.dart File pkg/analyzer-experimental/lib/options.dart (right): https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/lib/options.dart#newcode106 pkg/analyzer-experimental/lib/options.dart:106: * options/flags, this class can be replaced with a ...

7 years, 11 months ago (2013-01-22 19:16:36 UTC) #10

Message was sent while issue was closed.

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/...
File pkg/analyzer-experimental/lib/options.dart (right):

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/...
pkg/analyzer-experimental/lib/options.dart:106: * options/flags, this class can
be replaced with a simple [ArgParser] instance.
On 2013/01/22 18:31:53, pquitslund wrote:
> On 2013/01/18 22:25:22, Bob Nystrom wrote:
> > Mention #5262 here.
> 
> On the surface, that doesn't seem to capture this specific issue.  Or?  If you
> agree, I'll open another ticket and point this to it.
> 

I guess technically that bug is a little different, but ignoring unrecognized
options is somewhat related to it.

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/...
pkg/analyzer-experimental/lib/options.dart:111: final ArgParser _parser;
On 2013/01/22 18:31:53, pquitslund wrote:
> On 2013/01/18 22:25:22, Bob Nystrom wrote:
> > Is there a reason you're forwarding instead of inheriting and overriding?
> 
> Habit.  I generally avoid subclassing where it doesn't have clear benefits. 
> Blame my caution on the indoctrination of the Go4!

Well, they said *prefer* composition, not "never ever inherit". :)

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/...
pkg/analyzer-experimental/lib/options.dart:168: var filtered = <String>[];
On 2013/01/22 18:31:53, pquitslund wrote:
> On 2013/01/18 22:25:22, Bob Nystrom wrote:
> > return args.where((arg) => !arg.startsWith('--') ||
> >                            _knownFlags.contains(arg.substring(2));
> > 
> > :)
> 
> Cheeky! :)  But I think this is missing the parameter eating part.  That is, I
> think this should work for options as well.  Or?  IOW, seeing this
> 
>  --ignore me --but not any of me 
> 
> should yield ['--but', 'not', 'any', 'of', 'me'] assuming 'but' is an
understood
> option.
> 
> What do you think?

I believe this should do the right thing here. If it's not an option, (i.e.
doesn't start with "--") it will always get included in the result since
!args.startsWith('--') will be true.

If it is an option, it will only be included in the result set if it's a known
one.

pquitslund

7 years, 11 months ago (2013-01-22 23:30:01 UTC) #11

Message was sent while issue was closed.

Thanks for the continued feedback!

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/...
File pkg/analyzer-experimental/lib/options.dart (right):

https://codereview.chromium.org/11938028/diff/8001/pkg/analyzer-experimental/...
pkg/analyzer-experimental/lib/options.dart:168: var filtered = <String>[];
On 2013/01/22 19:16:36, Bob Nystrom wrote:
> On 2013/01/22 18:31:53, pquitslund wrote:
> > On 2013/01/18 22:25:22, Bob Nystrom wrote:
> > > return args.where((arg) => !arg.startsWith('--') ||
> > >                            _knownFlags.contains(arg.substring(2));
> > > 
> > > :)
> > 
> > Cheeky! :)  But I think this is missing the parameter eating part.  That is,
I
> > think this should work for options as well.  Or?  IOW, seeing this
> > 
> >  --ignore me --but not any of me 
> > 
> > should yield ['--but', 'not', 'any', 'of', 'me'] assuming 'but' is an
> understood
> > option.
> > 
> > What do you think?
> 
> I believe this should do the right thing here. If it's not an option, (i.e.
> doesn't start with "--") it will always get included in the result since
> !args.startsWith('--') will be true.
> 
> If it is an option, it will only be included in the result set if it's a known
> one.

Well fine.  Are you ALWAYS right? ;)

Follow-up here: https://codereview.chromium.org/12049028/

Expand Messages | Collapse Messages