Design and apply a proper interface and structure

[SleeplessByte]

As discussed in #16 this makes a few changes to separate all logic and make it easier to test the units, reason about the flow and implement new behaviour:

• Input: any source of input, implementations are FileInput, DirectoryInput and InlineInput. They can provide 0 to many source files to be parsed
• Output: an analysis output in the raw format, has toProcessable in order to turn it into something the official interface expects (a json formatted file)
• OutputProcessor: like a chain, calls the previous processor and can make changes, implementations are: LogOutput, FileOutput and a noop PassThroughOutput. I'd like this to be more like a middleware in the future.
• Analyzer: only has run(input: Input): Promise<Output>
• Runner: only has call(analyzer: Analyzer, input: Input, options: Readonly<ExecutionOptions>): Promise<Output>

The runner is just a simple way to take the ExecutionOptions and apply certain OutputProcessors or not, it's not longer vital to use at all.

Each Analyzer can act on an AST by parsing the Input they get via a parser. Each analyzer can define their own parser and options, but we provide sane defaults. The default parser uses @typescript-eslint/typescript-estree. The base implementation of the Analyzer no longer knows about parsing at all.

The new code simplifies a lot. Consider the new base execution:

const analyzer = new AnyAnalyzerClass()

run(analyzer, input, options)
  .then((output) => /* yay */ )
  .catch((err: any) => /* nay */ )

The default input is given by the command line:

// => options comes from the command line
// => exercise comes from the command line

const input = new DirectoryInput(options.inputDir, exercise.slug)

This also means that the default test is much easier:

const analyzer = new TwoFerAnalyzer()
const input = new InlineInput([solutionContent])
const output = await run(analyzer, input, options)

The runner can be replaced:

const analyzer = new TwoFerAnalyzer()
const input = new InlineInput([solutionContent])
const output = await analyzer.run(input)

Additionally the bin/batch(-runner) and bin/stat(istic)s scripts have been rewritten utilising the new structure.

[depsir]

Wow, great work!

So now the analyzers just take an Input which is in the end an array of strings (after the read method, which makes sense since we want to analyze a source code which is ultimately a bunch of strings) and then the analyzer is responsible to parse it as it sees fit, analyze the parsed structure and provide an Output, which is independent from the exercism api, the filesystem and everything.

So if I want to test an analyzer I could and should avoid to test the transformation from the raw Output to the formatted json output.

I definitely prefer the example test without the runner since every processor should be tested on their own, and then I could have one test that exercises the runner/processor composition.
But the majority of analyzer tests should focus on the production of the output, which is independent from the options or the processors.

I'm not sure if the Comment contains some sort of identifier of the comment type, e.g. "missing_parameter". I think that this is what I would like to test, along with the output status: more precise than the number of comments and not linked to the verbose template output.

something like (not actually working, just to express the idea)

    const solutionContent = `
    export const twoFer = () => {
      return \`One for \${arguments ? arguments[0] : "you"}, one for me.\`;
    };
    `.trim()

    const analyzer = new TwoFerAnalyzer()
    const input = new InlineInput([solutionContent])
    const output = await run(analyzer, input, options)

    expect(output.status).toBe('approve_with_comment');
    expect(output.comments.length).toBeGreaterThanOrEqual(1)
    // pseudocode assertion here:
    // expect comments to include one with type "missing_parameter"

Another benefit I see of this structure is that if should be easier to change the control flow from exception based to something else: it is well defined and clear that providing another base implementation of Analyzer we should be able to create analyzers with a different control flow, as long as the outer interface remains the same

I wonder if a default parser should be provided by the base analyzer but I guess we will see when we have more than two.

[SleeplessByte]

So if I want to test an analyzer I could and should avoid to test the transformation from the raw Output to the formatted json output.

Correct. That was my intention. ✔️

I definitely prefer the example test without the runner since every processor should be tested on their own, and then I could have one test that exercises the runner/processor composition.

Yes, that is what I envision for the analyzer-specific tests; the smoke test should use it though, in order to be e2e.

I'm not sure if the Comment contains some sort of identifier of the comment type, e.g. "missing_parameter". I think that this is what I would like to test, along with the output status: more precise than the number of comments and not linked to the verbose template output.

javascript-analyzer/src/interface.d.ts

Lines 23 to 28 in a6b4d9a

	interface Comment {
	message: string
	template: string
	variables: Readonly<{ [name: string]: string | undefined, [name: number]: string | undefined }>
	externalTemplate: string
	}

It does indeed! It's called externalTemplate and you can see it in the example below as 'javascript.generic.no_method':

javascript-analyzer/src/comments/shared.ts

Lines 3 to 5 in a6b4d9a

	export const NO_METHOD = factory<'method_name'>`
	No method called \`${'method_name'}\`. The tests won't pass without it.
	`('javascript.generic.no_method')

something like (not actually working, just to express the idea)

Actually, what you wrote works 😅 🚀

I wonder if a default parser should be provided by the base analyzer but I guess we will see when we have more than two.

Yeah, not an issue for right now indeed; it's on my backlog of things to consider 👏

[SleeplessByte]

Using the new interface, I've rewritten the batch-runner and statistics scripts. Yes, it's much easier to write now; so I've moved them to typescript as well.

Statistics

yarn build && bin/stats.sh -c two-fer

This now uses the same Bootstrap call as analyze meaning you can give it the same options. -d and -c as well as inputDir are respected. This means you can change the root fixtures dir by passing it in:

bin/stats.sh -c two-fer other/fixtures/dir

=> skipping C:\Users\Derk-Jan\Documents\Github\exercism-javascript-analyzer\test\fixtures\two-fer\15
   No input source files for two-fer

=> skipping C:\Users\Derk-Jan\Documents\Github\exercism-javascript-analyzer\test\fixtures\two-fer\154
   No input source files for two-fer

=> skipping C:\Users\Derk-Jan\Documents\Github\exercism-javascript-analyzer\test\fixtures\two-fer\217
   No input source files for two-fer

=> skipping C:\Users\Derk-Jan\Documents\Github\exercism-javascript-analyzer\test\fixtures\two-fer\23
   No input source files for two-fer

=> skipping C:\Users\Derk-Jan\Documents\Github\exercism-javascript-analyzer\test\fixtures\two-fer\405
   No input source files for two-fer

=> skipping C:\Users\Derk-Jan\Documents\Github\exercism-javascript-analyzer\test\fixtures\two-fer\41
   No input source files for two-fer

=> skipping C:\Users\Derk-Jan\Documents\Github\exercism-javascript-analyzer\test\fixtures\two-fer\466
   No input source files for two-fer

=> skipping C:\Users\Derk-Jan\Documents\Github\exercism-javascript-analyzer\test\fixtures\two-fer\54
   No input source files for two-fer

=> skipping C:\Users\Derk-Jan\Documents\Github\exercism-javascript-analyzer\test\fixtures\two-fer\58
   No input source files for two-fer

=> skipping C:\Users\Derk-Jan\Documents\Github\exercism-javascript-analyzer\test\fixtures\two-fer\141
  ',' expected. ({"index":16,"lineNumber":1,"column":16})

=> skipping C:\Users\Derk-Jan\Documents\Github\exercism-javascript-analyzer\test\fixtures\two-fer\219
   Declaration or statement expected. ({"index":121,"lineNumber":6,"column":4})

=> skipping C:\Users\Derk-Jan\Documents\Github\exercism-javascript-analyzer\test\fixtures\two-fer\429
   ')' expected. ({"index":30,"lineNumber":1,"column":30})

=> skipping C:\Users\Derk-Jan\Documents\Github\exercism-javascript-analyzer\test\fixtures\two-fer\441
   Declaration or statement expected. ({"index":178,"lineNumber":9,"column":0})

=> skipping C:\Users\Derk-Jan\Documents\Github\exercism-javascript-analyzer\test\fixtures\two-fer\88
   Declaration or statement expected. ({"index":0,"lineNumber":1,"column":0})

=> skipping C:\Users\Derk-Jan\Documents\Github\exercism-javascript-analyzer\test\fixtures\two-fer\90
   ';' expected. ({"index":39,"lineNumber":2,"column":26})

Raw output

{
  "invalid": 0,
  "valid": 485,
  "total": 485,
  "unique": 358
}

Parsing statistics

This is the number of unique Abstract Syntax Trees after stripping commentary,
location data (whitespace) and other tokens.

total	unique	valid	invalid
485	358	485	0

Batch runner (two-fer)

yarn build && bin/batch.sh -c two-fer

Raw

{
  "toolRuntime": "537ms",
  "refer_to_mentor": {
    "count": 103,
    "comments": {
      "unique": [
        "The string template looks incorrect. Expected a template with 3 components."
      ],
      "unique_templates": [
        "The string template looks incorrect. Expected a template with 3 components."
      ]
    },
    "runtimes": {
      "total": 29805802591,
      "average": 289376724,
      "median": 311634200
    }
  },
  "disapprove_with_comment": {
    "count": 373,
    "comments": {
      "unique": [
        "You currently use a conditional to branch in case there is no value passed into\n`twoFer`. Instead you could set the [default value](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters)\nof name to 'you' to avoid this conditional.",
        "In _JavaScript_, always prefer [strict (identity and non-identity) equality](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Comparison_Operators#Identity)\nsuch as `===` and `!==` over the forms that use implicit type coercion,\nsuch as [`==`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Comparison_Operators#Equality)\nand [`!=`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Comparison_Operators#Inequality),\nunless you explicitly want to coerce the type of one of the two operands.\n\nThere are definitely cases where you'll want to use non-strict equality, but\nthat's not the case in this exercise.",
        "You currently use a conditional to branch in case there is no value passed into\n`twoFer`. Instead you could set the [default value](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters)\nof X to 'you' to avoid this conditional.",
        "Instead of relying on name being \"undefined\" when\nno value is passed in, you could set the default value of 'name' to\n'you'.",
        "No method called `twoFer`. The tests won't pass without it.",
        "No [export](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/export) called `twoFer`.\nThe tests won't pass without it.\n\nDid you forget adding: `export twoFer`?",
        "You currently use a conditional to branch in case there is no value passed into\n`twoFer`. Instead you could set the [default value](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters)\nof n to 'you' to avoid this conditional.",
        "You currently use a conditional to branch in case there is no value passed into\n`twoFer`. Instead you could set the [default value](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters)\nof oneFer to 'you' to avoid this conditional.",
        "You currently use a conditional to branch in case there is no value passed into\n`twoFer`. Instead you could set the [default value](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters)\nof x to 'you' to avoid this conditional.",
        "Your function `twoFer` does not have a parameter.\nThe tests won't pass without it.",
        "You currently use a conditional to branch in case there is no value passed into\n`twoFer`. Instead you could set the [default value](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters)\nof value to 'you' to avoid this conditional.",
        "You currently use a conditional to branch in case there is no value passed into\n`twoFer`. Instead you could set the [default value](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters)\nof person to 'you' to avoid this conditional.",
        "You currently use a conditional to branch in case there is no value passed into\n`twoFer`. Instead you could set the [default value](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters)\nof who to 'you' to avoid this conditional.",
        "You currently use a conditional to branch in case there is no value passed into\n`twoFer`. Instead you could set the [default value](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters)\nof userName to 'you' to avoid this conditional.",
        "Instead of relying on who being \"undefined\" when\nno value is passed
in, you could set the default value of 'who' to\n'you'.",
        "You currently use a conditional to branch in case there is no value passed into\n`twoFer`. Instead you could set the [default value](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters)\nof someName to 'you' to avoid this conditional."
      ],
      "unique_templates": [
        "You currently use a conditional to branch in case there is no value passed into\n`twoFer`. Instead you could set the [default value](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters)\nof %<parameter>s to 'you' to avoid this conditional.",
        "In _JavaScript_, always prefer [strict (identity and non-identity) equality](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Comparison_Operators#Identity)\nsuch as `===` and `!==` over the forms that use implicit type coercion,\nsuch as [`==`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Comparison_Operators#Equality)\nand [`!=`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Comparison_Operators#Inequality),\nunless you explicitly want to coerce the type of one of the two operands.\n\nThere are definitely cases where you'll want to use non-strict equality, but\nthat's not the case in this exercise.",
        "Instead of relying on %<maybe_undefined_expression>s being \"undefined\" when\nno value is passed in, you could set the default value of '%<parameter>s' to\n'you'.",
        "No method called `%<method_name>s`. The tests won't pass without it.",
        "No [export](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/export) called `%<export_name>s`.\nThe tests won't pass without it.\n\nDid you forget adding: `export %<export_name>s`?",
        "Your function `%<function_name>s` does not have a parameter.\nThe tests won't pass without it."
      ]
    },
    "runtimes": {
      "total": 106715240198,
      "average": 286099839,
      "median": 296772301
    }
  },
  "approve_as_optimal": {
    "count": 7,
    "comments": {
      "unique": [],
      "unique_templates": []
    },
    "runtimes": {
      "total": 1867306301,
      "average": 266758043,
      "median": 295006101
    }
  }
}

Statistics

Status	Count	Comments	Unique	Avg	Median	Total
Approve (optimal)	7	0	0	257ms	286ms	0s
Approve (comment)	0	0	0	0ms	0ms	0s
Disapprove (comment)	373	16	6	275ms	296ms	10s
Refer to mentor	103	1	1	279ms	307ms	2s
Total	483	17	7	275ms	300ms	13s

[SleeplessByte]

I will merge this over the weekend if there are no strong objections. I want to make sure this is ready sooner rather than later.

cc @depsir @tejasbubane cc-ing you as I always value your thoughts.