html reports

[voytech]

Have You ever been considering generating html reports ?

[tolitius]

that could be quite useful, PR is welcome :)

[voytech]

I have forked Your repository and created reporting using hiccup. I have also made some other improvements (in my optinion). Below short list of what I have done:

• Created boot-check-report - which will generate html report with issues. boot-check-report uses fileset tempdir from boot to read issues (as input-file) reported (persisted) by other checker tasks

• Added throw-on-error also as a separate task to enable aggregated issues exception (You may e.g. want to first print report, then throw exception with all issues found by all checkers in boot pipeline - this exception will stop rest of pipeline - currently You will stop pipeline after first checker which reports something, and other checkers will not execute - this allow to throw after all checker are executed )

• Also in boot-check.clj I have extracted some repeatable code in each of checker task into shared function.

• I have updated also README with above changes.

If You wish You can look at my fork, It is still WIP (I have to resolve issues reported by checkers on my code yet ;) ) This is my ( potential ;) ) first contribution ( as PR ) - please let me know if You want to use something - maybe everything ? if not - I can select what You want and make PR.

Forgot to add important note:
Html reporting is not one to one with what is printed by checkers on stdout. This is because e.g. bikeshed doesnt return all warnings - but rather summary. Kibit returns warnings but it seems there is bug because filename component is missing in returned map. (I think I have some workarounds for above in my mind - but this is rather ugly - and it will be better to create PR for bikeshed and kibit in nearest future ;) )

[tolitius]

wow, that is a lot of work :)

given how much different it is I would not want to merge all the changes.

I really like the HTML reports, so we can start with incorporating that. Does this file encapsulate all the reporting logic, or there are other pieces / hooks? In other words, if we were to merge HTML report feature only, what would need to change?

Also in boot-check.clj I have extracted some repeatable code in each of checker task into shared function

The reason I have some code duplication is because I do prefer decoupling to reuse, this is just from my experience creating and maintaining software. I am not insisting this is the only / right way.

We don't need a separate task for "boot-check-report". This could be passed in options, unless I am missing something.

I do like the idea about a separate "throw-on-error" task. It does make sense have an option to accumulate exceptions and wait until all the checks are done before throwing them.

Unless I really need them I usually try to avoid multi methods since they are painful to iterate with at dev time (in REPL). There is of course a big theoretical debate about records vs. just maps, but I also prefer maps since, in my opinion, they do not couple data and type. This is just so you understand where I stand.

[voytech]

Yes so, HTML report task is using tmpdir and file which is incrementally growing with issues while different checkers reports issues in separate tasks. I think it should be separate task which agregates issues from checkers. If we use just option for linters - say - r - generate-report we will end up with report per checker task. To be able to aggregate all warnings within single report we need a separate task which uses that incrementally Updated issue file (by all checker tasks within pipeline ). I do not see ant other possibility to aggregate all issues within single report but I might be wrong.

I do not know what timezone You are, but here in Poland it is night - I need to go śleep right now, but tomorrow I will elaborate more on this.

I think the best would be to start with HTML reports incorporation as You suggested - but it should match your conventions as much as possible - as boot check is your project ;) and I agree with that totally ;)

[voytech]

Here is how it looks like (just in terms of HTML report support)

1. Each checker has been modified so that it returns vector of issue records (bikeshed and kibit have some problems in this area - I am creating PR
   s to solve that)
2. Report method used by reporting task

• It is a multimetod - someone may want to create own reporter in his project. Someone may want to E.g write warnings to db or send on server.
• it accepts vector of issue records
• it writes report.html into tmpdir output (may be served by serve task potentially ) and to current dir.
• report task reads vector of issues from tmpdir input-files - Thanks to that it can be separate task which focus only on reporting.
• currently no switch option to allow each of checker task to print reports by themselves.

How it can be changed to better fit your conventions ????

• get rid of separate report task !!!
• add same reporting capability to every checker task in following way:
  • check if flag -gen-report is present,
  • if present, read tmpdir issues file as other checkers may have reported sth, run checker, append issues write to tmpdir issues file, generate or override existing report file ;)
  • if not present - do nothing in this area ;)
  • I can also move from defrecord to map - then file model.clj not needed anymore ;)
  • I can change report multimethod to be just func and loose flexibility of registering different reporters. In fact it would follow yagni rule then i think. I am not convinced of value added by adding possibility to register new reporters - it is like making this project to be extended reporting functionality in scope of other project which uses boot-check - but maybe boot-check should be self-contained and complete. And that is it. Maybe if there is some reason to add abother reporting then let someone fork and make PR to improve boot-check then.

[voytech]

Of course I am ready to make those changes. In fact I now feel much more convinced towards the idea of getting rid of separate report task and adding option -gen-report - even it seems to be more flexible - now Each checker is adding issues to issue tmpdir file. It will be used only if separate boot-check-report is at the end. By removing boot-check-report and adding options we may even say that some of checker should not add issues to report (only print stdout ) ;) ;)

[tolitius]

@voytech yes, I think incremental changes work best, and I appreciate you being so enthusiastic about it :)

the ideal scenario is to have the existing boot check code with minor changes:

• each checker will have an optional :report option i.e.:

(check/with-yagni :options {:report {:kind :html}
                            :entry-points ["test.with-yagni/-main"
                                           "test.with-yagni/func-the-second"
                                           42]})))

if this option is there, it would create an HTML report, if the :report option is not there, it would write the report to stdout. i.e. :report {:kind :stdout} is the default.

This way different kinds of reports could be added later when/of some needs it.

• a separate tolitius.report package that would have a generator.clj.

It would have "generate" multi methods that would dispatch on (-> checker :options :kind). I agree that using multi methods for potentially different kinds of reports is a good idea, since the benefit outweighs the dev inconvenience.

This way it would be a lot simpler to integrate to an existing codebase: 99% of the logic would live inside generators, and the remaining 1% would go towards handling :report {:kind :xyz} option at the checker level.

[voytech]

@tolitius That is all doable and reasonable for me ;). But one question yet needs to be answered ...
If You prefer to use multimethods (to enable custom reporters) for generators and using following options: {:report {:king :html}} is your preferred way of incorporating this.. then - how do we decide what reporter to use (in aggregate report file) when multiple checkers in the pipeline are using reporting eg:
...
(check/with-yagni :options {:report {:kind :html})
(check/with-kibit :options {:report {:kind :xml}) <--- xml is fiction here ;)
...

• should we have separate report file per checker ? (with issues only reported by particular checker).
  In above scenario we would have e.g. yagni-report.html and kibit-report.xml ;)
• should we have aggregate report file for all checkers (with all warnings reported) invoked with :report switch option ? - but if so what reporter (xml? html?) to use for aggregate report ? ;)

Personally I would better like to have aggregate report, because it is better from consumer perspective to see all issues reported within one place (If there are many reports - user needs to switch between browser tabs e.g so see if everywhere it is cleared ;) ). Also I would not allow to pass option :report with reporter type - but just single flag specifying that report should be generated for specific checker e.g - :gen-report true. To choose reporter I would use set-env! within task ? (I am not sure if it can carry custom data) - and that would allow to apply choice globally in main task - and each checker task would then know what reporter to use.

Let me know how You want it. It is your choice - when I know that - I can make this minimal set of changes and make PR ;)

[tolitius]

Personally I would better like to have aggregate report
{:gen-report true}

yes, I agree, no need for confusion between different checkers: one report for all with an optional inclusion is better idea.

If you have difficulties figuring out where to pass a report format from, make it HTML by default, and later we can figure out how to pass a different format in. Since HTML would be mostly welcomed and used I believe (at least at first).

[voytech]

@tolitius I think a can make a PR. I have intoduced in my judgement very minimal set of changes:

• added some functions in boot_check.clj - to allow persisting of warnings as reported by checkers (and to load warnings also for further processing)
• made modifications in each checker - as they need to return vector containing canonical issue maps - to be understood by reporter.
• in check.clj - I have got rid of records. I am using maps instead.
• of course html.clj - default implementation of reporter.
• also modified README.

That is still lot of changes, but IMHO this is a minimum to allow reporting.

Also I started contributing in dependent checkers :) :
-kibit - I have already provided a fix to include filenames toghether with issues when returning results. It has been merged to master already and next release will contain that fix.
-bikeshed - Here I am just starting. I want to make it return vector of problems so it can be used in reporters.