-
Notifications
You must be signed in to change notification settings - Fork 6k
Refactor args parsing/environment constructor for scenario_app
#50980
Refactor args parsing/environment constructor for scenario_app
#50980
Conversation
gaaclarke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
| exitCode = 1; | ||
| return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that uncaught exceptions will also set the exit code. Can we rely on that instead of setting the exitCode and trying to short circuit the logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a caught exception though.
I could rethrow but then the output will look meh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ugly but informative for developers since it will have the stack trace. I ended up going the uncaught route since trying to manually short circuit and maintain a global variable (exitCode) seemed to be fighting the language. Your call. I may even have ended up doing it both ways in different places =P
| outDir: Directory(options.outDir), | ||
| adb: File(options.adb), | ||
| smokeTestFullPath: options.smokeTest, | ||
| useSkiaGold: options.useSkiaGold, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use spread operators for named arguments? bummer.
Filed issue, looks like the spread operator isn't documented on the operators page: dart-lang/site-www#5599
This moves the ever-growing amount of options and defaults into it's own class(es).
The test runner itself has no tests (yet), but this shim will make writing tests easier. I tried to make no other real changes to how the runner functions in this PR.