API for reek v3: The first proposal

[troessner]

I thought about our future API for reek 3. I believe that this: https://github.com/troessner/reek/blob/develop/docs/API.md is already a good start.

What I had in mind:

[1] Getting started

Set up

require 'reek/api'

This will make available:

• All the reporters (see below)
• The Examiner

Usage

1.) Create a reporter. Can be:

• TextReport
• YAMLReport
• JSONReport
• HTMLReport
• XMLReport

reporter = Reek::CLI::Report::TextReport.new

2.) Create an examiner:

examiner = Reek::Examiner.new(source)

where source can be a File, a string or an array (so basically like it is right now)

3.) Add the examiner to the reporter:

reporter.examiners << examiner

4.) Print the result:

puts reporter.show

Exposed API methods:

• Reporter#show
• Reporter.examiners <<
• The initalizers of the reporters and Examiner

Necessary changes

• Move Reek::Core::Examiner to the top level so it becomes Reek::Examiner
• Create reek/api
• Change Reporter.add_examiner to Reporter.examiners <<

Spec API

Our spec matchers described here should stay like they are.

[2] Outlook

In the next step I would add a none breaking change that

1.) exposes our AppConfiguration
2.) exposes our SmellRepository
3.) makes the API more convenient to use like this:

report = Examiner.new source: ,
                      reporter:         TextReport.new,
                      smell_repository: SmellRepository.default,
                      configuration:    AppConfiguration.default
report.show                      

This would obviously be a lot harder than [1] since AppConfiguration would be settable on an examiner base. So I'd like to go with [1] to keep things easy and get to a stable version quick and then put out [2] as an add-on.

What do you guys think?

[chastell]

I like it in general, but I’m unsure about exposing all of the Reek::* plumbing.

I’d consider wrapping the API calls in thin Reek.foo (or Reek::API.foo) methods – this way we’ll be able to refactor things without having to maintain backward-compatible constants:

require 'reek/api'
reporter = Reek::API.reporter(:text)
examiner = Reek::API.examiner_for(source)
reporter.examiners << examiner
puts reporter.show

Eventually we should probably consider using core Ruby object types (Strings, Symbols, Arrays, Hashes) for communication over the API boundary:

report = Reek::API.report(source:   source,
                          reporter: :text,
                          smells:   %i(attribute data_clump nil_check),
                          config:   config) # a path to a YAML file or a Hash
puts report.show

[mvz]

I like the idea of just exposing a set of methods on Reek or Reek::API to create objects that may change names in the future.

We should also determine the set of documented methods on Examiner. Lookin at pronto-reek, Examiner#smells should at least be exposed. Which of course leads to the need for a stable API for SmellWarning.

I agree that making the configuration injectable can be a next step.

[troessner]

Eventually we should probably consider using core Ruby object types (Strings, Symbols, Arrays, Hashes) for communication over the API boundary:

Good idea!

[troessner]

Reek::API.report

I'd rather not have Reek::API but just requiring "reek/api" that sets up an Examiner.
My reasoning here is that Reek::API doesnt mean anything. We could call it "Reek::Foo" and it would have the same expressiveness.
Let's rather stick with "Reek::Examiner" and make that as user-friendly as possible.

[chastell]

Let's rather stick with "Reek::Examiner" and make that as user-friendly as possible.

Ok, but let’s require 'reek/examiner' rather than require 'reek/api' in this case. :)

[guilhermesimoes]

2.) Create an examiner:

examiner = Reek::Examiner.new(source)

where source can be a File, a string or an array (so basically like it is right now)

Please make it so that source can also be an AST :)

[troessner]

Good idea @guilhermesimoes !

[mvz]

2.) Create an examiner:

examiner = Reek::Examiner.new(source)

where source can be a File, a string or an array (so basically like it is right now)

I've always found these options for source a bit confusing, especially how a string is interpreted as code but the array as list of file names. Internally, we only ever pass a single SourceCode object. How about allowing a single piece of source only, which can be:

• An IO object (usually File)
• A string containing source code
• An AST
• (Used internally, and we can probably refactor this case away) a Source::SourceCode object.

What do you think?

[troessner]

Yes, 👍 for that. The array option is confusing.
Also 👍 for the AST..:)

[mvz]

Allowing an AST will be for a future, backwards-compatible, update to the API.

[mvz]

I've considered this kind of setup:

report = Reek::API.report(source:   source,
                          reporter: :text,
                          smells:   %i(attribute data_clump nil_check),
                          config:   config) # a path to a YAML file or a Hash
report.show

However, I feel this ties together too much at the moment. I prefer exposing just the two pieces: examiner and report.

I like @chastell's version with wrapper methods. Perhaps like so:

require 'reek'
reporter = Reek.reporter(:text)
examiner = Reek.examiner_for(source)
reporter.examiners << examiner # or add_examiner
reporter.show

However, we will still be exposing Reek::Examiner and the reports as visible class, so I don't see a problem with exposing their initializers as well.

[chastell]

I’m still not sure why the users need to be concerned with the fact that we have reporters and examiners. They want a report for their source – what we use internally shouldn’t leak outside.

I don’t mind people providing their own examiners if that’s a use-case we want to support, but as an option with a good default, not a required step of obtaining a report for the given source.

The problem is, of course, which parts should be easily tweakable (smells? config?) and how. :)

[mvz]

I’m still not sure why the users need to be concerned with the fact that we have reporters and examiners.
They want a report for their source – what we use internally shouldn’t leak outside.

Some of our users don't need a report, they just need the smells.

[chastell]

Good point, and we should expose them – but we should expose them in a way that is as generic as possible, so a #smells call on the report(er) would make sense to me.

[mvz]

I think the question is if we're exposing a shrink-wrapped API for external users, or we're just exposing a cleaned-up version of what we use internally. The implementation I made is definitely of the latter kind.

The former type of API will require a lot more thought:

• How are we going to allow people to investigate more than one source?
• Do we want to expose the option of examining some sources with one set of smells, and other sources with another set?
• Do we want to force people to choose a reporter when they just want the smells?

Also, such an API requires more maintenance, since we may not be using the API ourselves.

For our internal use, I like the split between examiner (finds certain smells in a source) and report (displays found smells in a nice way).

I'm not against a more cleaned-up API that exposes less internals, but I think we should first get some experience with how the current classes are used before settling on a design.

[chastell]

Ok, I agree on the reasoning and step-by-step approach. Let’s go with exposing (and maintaing) our more internal API in reek 3, come up (over time) with a more generic (based on Ruby core types, etc.) API in the meantime and if it turns out maintaining the exposed API compatibility is problematic we can alter/deprecate it in reek 4.

[mvz]

👍

[troessner]

Ok, I agree on the reasoning and step-by-step approach. Let’s go with exposing (and maintaing) our more internal API in reek 3, come up (over time) with a more generic (based on Ruby core types, etc.) API in the meantime and if it turns out maintaining the exposed API compatibility is problematic we can alter/deprecate it in reek 4.

👍

[troessner]

Merged, closing this.