CommandLineAnalyzer interface change

[frerich]

CommandLineAnalyzer interface change

[frerich]

It's kinda hard to discuss the merits of making this more object-oriented without actual code, but at this point I'm very skeptical that this will be an improvement. Addressing your motivations for bringing this up:

Since there is no internal state (= no members), the options variable is passed around all the time.

Actually, only _preprocessToStdout and _outputFileFromArgument use the options argument. _preprocessToStdout is an unused method and _outputFileFromArgument could be refactored to not need options without too much trouble. Actually, there is only one caller of _outputFileFromArgument, so it might be viable to just inline the call.

The name "analyze" is not helping a lot.

I agree, but having the term 'analyze' in a call FooAnalyzer.analyze(cmdLine) seems as useful (i.e. not very much) to me as writing CommandLineAnalysis(cmdLine). Making this object-oriented doesn't magically make it more expressive. I believe an actual improvement would be to be more explicit about what the function does: it deduces the source and output files given some command line.

The output if analyze() is an unnamed tupel[sic!]

In this particular case, I see no issue with having a function return a tuple. It actually permits assigning names to the tuple values on the callsite, i.e. given def f(): return 47, 11 you could write foo, bar = f(). If instead you return an object, you write o = f() and then have to use o.foo and o.bar -- an unnecessary indirection.

We cannot test the content of options

We can, this would just be a matter of calling CommandLineAnalyzer._parseOptionsAndFiles. It would be fine to make this public (i.e. by removing the leading underscore) for this purpose. In general, having free functions whose behaviour is only defined by their arguments (i.e. they are "pure" and free of side effects) is ideal for testing. No setup or teardown work is required, no objects to be greated. Just call a function with arguments and check the output.

static functions cause long caller code (e.g. CommandLineAnalyzer._outputFileFromArgument)

You don't need to reference the class object when calling static methods (this is only true for Ruby, not for most other languages like C++ or Python). The code could just as well use self._outputFileFromArgument. It is however not uncommon to see fully-qualified calls to express that the called method does not depend on the member variables but only on globals and arguments.

IIRC, the code currently only uses a class CommandLineAnalyzer to implement local functions. A simpler way to achieve this might be to just introduce a new file (say, cmdline.py) which exposes individual functions (preferably with short names) related to handling the command line, e.g.:

• cmdline.expand (corresponds to current expandCommandLine function) which recursively expands command files. It takes a string list corresponding to the argument vector and returns a new string vector. expand is hence idempotent.
• cmdline.parse (corresponds to current CommandLineAnalyzer._parseOptionsAndFiles) takes care of parse an argument vector (i.e. a string list) into a dictionary of options and a list of source files.
• cmdline.getInputOutputFiles (corresponds to current CommandLineAnalyzer.analyze) is the main entry point. It takes an argument vector, expands it, parses it and then determines the input and output files involved in the invocation.

[webmaster128]

Okay, fine. There is no real need to change it, I just thought it makes a bunch of places slightly better. But I am perfectly fine keeping it as it is :)

Let me just answer a few things:

• I did not see that _preprocessToStdout is unused, PR is out.
• Inlining _outputFileFromArgument is good. It makes the code more concrete.
• Making _parseOptionsAndFiles public is okay. I would avoid testing private interfaces, which is considered bad design.
• I had the impression that you dislike the word "analyze()" more than me ;)
• I don't think it's a good style to call static functions in a member fashion. But the longer names don't hurt, right.
• I would like to avoid splitting the project in multiple files as long as possible. It is very handy with just over 1000 lines to have everything in place. More files mean more license headers and linting. Sure, it can be done if really useful, but I don't see that.

[frerich]

I guess there's no action to be done here, so let's close this issue.