Please wait on doing any big merges until the Script LIbrary PR goes in.

Incidentally, -version and -help now execute instantly (previously each had a 2 second delay on my machine) since they are now effectively a no-op with regard to the application.

Help output is also much nicer, since the extra line breaks have gone away:

I rebased this after the script libraries changes went into dev (there were no conflicts)

This is good to go now.

rebased, FWIW

@scriptcs/core it would be great if someone could have a look at this today. I'm flying to Portugal tomorrow afternoon and I'll be largely dark for a week.

It's a big change set, but each commit is very focused so reviewing one commit at a time is probably going to be easiest and quickest.

I'd love to see this go in. It's a massive simplification of the entry point to the app.

does it handle stuf like scriptcs -install foo 4.0.5 ?

does it handle starting REPL with log level set?

and a non-educated comment - you have deleted lots of tests; I understand the corresponding classes are gone, but do we no longer need to check the scenarios they covered?

Guys, thanks very much for reviewing! It was nice to come home this evening to all these comments.

In response:

@filipw

does it handle stuf like scriptcs -install foo 4.0.5 ?

Yes, that is handled inside ScriptCsArgs.Parse() and is unit tested.

does it handle starting REPL with log level set?

Yes, this has been handled in CommandFactory since #869 went out in 0.13. That code has not been changed.

and a non-educated comment - you have deleted lots of tests; I understand the corresponding classes are gone, but do we no longer need to check the scenarios they covered?

There is way less code now and it is much simpler. The work now takes place in three places:-

• ScriptCsArgs.Parse() - the only meaningful thing that happens inside here beside wrapping around PowerArgs.Args.Parse<T>() is the special casing for -install to allow it to take a package name and a version. This is unit tested in ScriptCsArgsTests.
• ConfigMask a trivial type with a static factory method and a File.Exists wrapper around a JSON.NET deserialization call
• Config.Apply() applies a ConfigMask to a Config and returns a new Config

I think all but one of the scenarios which these types affect are covered in the acceptance tests. The only one which isn't, is the masking of CLI args over local and global options files and, come to think of it, that acceptance test should probably be added. My general approach to testing is to ensure the important scenarios are covered by acceptance tests and only to drill down into 'unit tests' when it's felt some particular implementation detail is risky enough to require it. That's why I included a unit test for ScriptCsArgs.Parse(). This is really the only complicated bit of logic that has survived from the original implementation, the rest is rather trivial.

@glennblock

This block of code that parses args should be in its own function and called by main. Makes the code easier to maintain.

Personally I would leave it where it is since it's trivial and single use. I only tend to encapsulate something if it's complicated or reused. If feelings are strong on this, I can encapsulate in a function.

@glennblock sure, I meant function too, 'encapsulate' was the wrong word 😄

So to paraphrase what Main is doing:

• divide the args into those for me and those for the script
• read the args intended for me (if I can't understand them, show what I do understand and return an error code)
• if you want help, show it and return a success code
• if you want to know my version, show it and return a success code
• if you want to load a config file which doesn't exist, refuse and return an error code
• create a config based on what I've been told
• load configs from the global and local files and mask the local config over the global config and my config over that
• create a command factory, feed the config into it and execute any resulting commands

Which part(s) do you think should move to a function?

Lines 18 to 35, everything related to parsing the args
On Sat, Mar 7, 2015 at 5:21 PM Adam Ralph notifications@github.com wrote:

@glennblock https://github.com/glennblock sure, I meant function too,
'encapsulate' was the wrong word [image: 😄]

So to paraphrase what Main is doing:

• divide the args into those for me and those for the script
• read the args for me (if I can't understand them, show what I do
  understand and return an error code)
• if you want help, show it and return a success code
• if you want to know my version, show it and return a success code
• if you want to load a config file which doesn't exist, refuse and
  return an error code
• create a config based on what I've been told
• load configs from the global and local files and mask the local
  config over the global config and my config over that
• create a command factory, feed the config into it and execute any
  resulting commands

Which part(s) do you think should move to a function?

—
Reply to this email directly or view it on GitHub
#944 (comment).

Parsing the args is already encapsulated in ScriptCsArgs.Parse() (second point above).

Tell you what, if this is the only thing you'd like to change, perhaps you can merge this PR and send your proposed refactoring in a new PR?

@filipw I've added Configuration scenarios to the acceptance tests

I am not finished reviewing. As to the refactoring it is reducing the code in Main and making it more readable. Extract Method is a very common pattern. The lines I mentioned can be easily ripped to a parse method.

Sure, I'm familiar with Extract Method 😉.

I guess it's a matter of taste. IMHO the lines should stay where they are. The key aspect is the return codes. There are a few cases in the method which Main needs to check and return a 0 or 1 appropriately. I wouldn't move these elsewhere as I think that will make the code less readable, since you'd have to propagate the return codes back up the stack.

There is actually only one line which does arg parsing, and that is line 22. The heavy lifting there is already extracted into ScriptCsArgs.Parse().

The only lines which I think could be moved elsewhere are:

• 16 and 17, but this would come with the overhead of either creating a return type, returning a Tuple<T1, T2> or using out parameters. All of these options I think would bloat the code for little benefit
• lines 49 to 52 - this is the loading and masking of the local and global config. Although it's 4 lines, it's a single statement with method chaining and I think it's very easy to read so I'd probably leave as is.

Ultimately Main is only 46 lines long and, as the entry point, it needs to orchestrate according to the CLI args with the constraint of returning status codes. In that context, I think it's as decomposed as is practical.

hold until post 0.14

0.14 has been released, so this PR is good to go again.

hold off - see #990

@glennblock Program.Main is now smaller 😉

This is good to go again.

👍

Great work, much more simplified!