Reporting

[JakeGinnivan]

Symmetric Test with approved exceptions:

Normal convention with approved exceptions

Failed convention

Only text reporting is available at the moment, but it is pluggable, and wanted to get feedback on many of the changes before writing more renderers

Also, I hate the naming on some of the interfaces. But can fix that later

[JakeGinnivan]

Record of all reports which have run, this will basically cause say an HTML report to be regenerated each time a new convention test is run. By keeping this list, we can just add to it, then pass the collection to the html renderer

[JakeGinnivan]

[kkozmic]

So I finally got around to looking through the PR (sorry it took so long).

I like the behaviour of it. I'm wondering though, if we're not trying to bite off too much in the initial run.
Mostly I'm concerned by the proliferation of interfaces it caused, and how much knowledge/helper logic required by reporting ended up leaking into convention and data classes.

Let me try spiking something to see if I can keeping it a bit more isolated from the core convention validation logic

[JakeGinnivan]

The conventions are actually simpler, before the report formatting lived in the convention, now it lives in the data class which makes more sense, as there will be more data than conventions so we do not have do duplicate that formatting logic as much

Most of the complexity came from supporting symmetric conventions

[kkozmic]

yeah I agree that formatting in conventions is less than optimal. I think it should be taken out of both convention and data into another abstraction(s).

Have a look at #23

I added two new abstractions, one for splitting the data, and the other for putting it in the desired format. This is just the first iteration (don't merge the PR just yet) to get some feedback.

Nice thing about it IMO is, it won't require any new interfaces or complication to data/convention classes.

Let me know what you think.

[JakeGinnivan]

One thing I liked about the reporting changes is that ConventionResult went away. I think that is a good thing, as it allowed conventions to simply return failures and you don't lose that granularity.

I might steal an idea or two from this, and apply it to my PR to simplify things.

Thinking about Convention.Formatters, which will be a collection of formatters. Your PR cannot handle formatting of Projects/Project etc yet, so not sure where that would live

[JakeGinnivan]

Updated the PR, this puts IConventionData back the way it was, same with IConvention (apart from the Title and Description properties)

[kkozmic]

Nice.

I like where this is going. It looks much simpler now. I'm not sold on getting rid of ConventionResult though. While it wasn't exactly the pinnacle of software design it had a few advantages IMO:

• better encapsulation of conventions. The whole tell don't ask thing which allowed the convention classes to be more flexible and more in control of what was happening.
• it meant we didn't need interface for symmetric conventions. One advantage that has is that a single convention class could be used as either symmetric or normal convention based on, say, a parameter.
• it likely would allow us to introduce changes in a non-breaking manner. Since a lot of the logic would be encapsulated in the convention classes we would be less likely to change the IConvention interface.

We ended up shifting a bunch of what it used to do to ResultInfo anyway, and while it made the convention classes a bit more straightforward to write, it also IMO removed a bit of power and control from them.

I'd be in favour of bringing it back.

[JakeGinnivan]

Actually, the reason I removed it was because of symmetric conventions. Because essentially you have two results shoved into one.

The other option was IConvention.Execute returns IEnumerable<ConventionResult>

[kkozmic]

Yes, you have two results in one, but this fact would be encapsulated by the ConventionResult itself. In the end, you still do that in a single test, so it's a single pass/failure.

For reporting purposes you could have that done in two sections by encapsulating it nicely in a ConventionResult method if you wanted to

[JakeGinnivan]

I actually prefer it without it. I found it got in the way, and the call to ConventionResult.For could be replaced by returning a list of failures, and exposing a title property.

It probably could be re-introduced now things have stabilised in the reporting. Did you want to have a go (so you can get a better feel for how it impacts the code).

[JakeGinnivan]

Also, by encapsulating it in the convention result, I found there was logic elsewhere which switched on something like IsSymmetric result.

Or, ConventionResult contains possibly multiple results. Introducing the interface for Symmetric Convention felt nicer (I played with a few of these options while doing the reporting stuff)

[kkozmic]

So the first part of your last message suggests you're against ConventionResult and the second part that you're for bringing it back. I'm confused :)

[JakeGinnivan]

I don't mind seeing what introducing it does, but atm I don't think it is worth it

Happy to see the result though, as you might have better ideas how to solve that issue

[kkozmic]

ok, let's merge this PR and I'll see where I can take it next.

I'm not sold that having ISymmetricConvention is a better solution, but let's see discuss this when we have some actual code to talk about alternatives