Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discussion: Structure, Flow, Architecture #16

Closed
SleeplessByte opened this issue May 27, 2019 · 1 comment
Closed

Discussion: Structure, Flow, Architecture #16

SleeplessByte opened this issue May 27, 2019 · 1 comment

Comments

@SleeplessByte
Copy link
Member

Now that we have some tracking on this repository, I'd like your input on going forward.

Current architecture

The initial architecture is mostly matching the ruby-analyzer, which means it's currently using exceptions as control flow. I'm personally not a fan, but for the use-case it works quite well. The interface API is extremely small:

  • approve: instantly mark as approved
  • dissapprove: instantly mark as dissapproved
  • redirect: instantly mark as "need human eyes"
  • comment: add a comment

I think it's also better than a pure functional approach as follows, because the example below quickly becomes as cognitively complex as the exception flow as soon as you need to nest rules, with the added bonus that if you forget a return, everything breaks (but might not actually error the program):

function execute(program) {
  if (!hasNamedExport(program)) {
    return new Output('dissapprove', { comments: [CommentFactory['named-export']] })
  }

  if (anotherCheck(program)) {
    if (!nestedRequiredment(program)) {
      return new Output('dissapprove', { comments: [CommentFactory['nesting']] })
    }
    if (isMinimal(program)) {
      return new Output('approve', { comments: [CommentFactory['tip-remove-nesting']] })
    }
  }

  ...
}

I do like the way the comment system is set up. It will give reasonable types which actually work, as soon as you give it one parameter (doesn't work with 0 parameters, yet). basically this is supposed to make sure the comments work even when they are extracted to the website-copy repo.

Work in progress

I have started extracting functions from the logic. These composable parts should help you write analyzers. They can do things like, but not limited to:

Additionally I've started on functionality where we can output equivalent code but change the output. The reason for this is we might want to give back alternatives using the solution's code without having to search for it (using location parameters, which can be buggy). For example you can fetch the parameter name of a function, and re-use that in the comments.

Preferable alternative?

The csharp-analyzer has a nice approach where:

  • each exercise gets its own ExerciseSolution class which is constructed with the Program;
  • properties of the program are defined on the ExerciseSolution so the "properties" live in one place
  • each exercise gets its own ExerciseAnalyzer` class (same as Ruby, JavaScript, TypeScript)
  • the analyzer has the flow and logic of which properties lead to which commentary.

This would basically be what we currently have, but refactor out solution properties from the logic. This would also remove most if not all "instance variables" from the analyzer (which I think is a win).

What's the gist?

Personally I would like to keep two-fer as is, but try out new patterns when tackling #13 and #14. Your thoughts and ideas are highly appreciated!

SleeplessByte added a commit that referenced this issue May 30, 2019
SleeplessByte added a commit that referenced this issue Jun 4, 2019
SleeplessByte added a commit that referenced this issue Jun 4, 2019
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.

Additionally the bin/batch(-runner) and bin/stat(istic)s scripts have been rewritten utilising the new structure.
@SleeplessByte SleeplessByte pinned this issue Jun 8, 2019
@SleeplessByte
Copy link
Member Author

I've tried out an isolated runner (that is analyzers without state except for parameters and return values) in

export class GigasecondAnalyzer extends IsolatedAnalyzerImpl {
protected async execute(input: Input, output: WritableOutput): Promise<void> {
const [parsed] = await Parser.parse(input)
// Firstly we want to check that the structure of this solution is correct
// and that there is nothing structural stopping it from passing the tests
const solution = this.checkStructure(parsed.program, output)

And I don't hate it.

It separates the properties of the solution from determining what the properties mean.

// ◼ * ◼
if (isBinaryExpression(expression, '*')) {
// 1e9 * ◼
// 10 ** 9 * ◼
// Math.pow(10, 9) * ◼
const isOptimisedGigasecondLeft =
isLiteral(expression.left, undefined, '1e9')
|| (isBinaryExpression(expression.left, '**') && isLiteral(expression.left.left, 10) && isLiteral(expression.left.right, 9))
|| (isCallExpression(expression.left, 'Math', 'pow') && isLiteral(expression.left.arguments[0], 10) && isLiteral(expression.left.arguments[1], 9))

Also allows for separation over multiple modules (which can individually be tested).

@SleeplessByte SleeplessByte unpinned this issue Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant