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

Adding html reporting functionality #17

Merged
merged 10 commits into from
Feb 15, 2018
Merged

Conversation

voytech
Copy link
Contributor

@voytech voytech commented Feb 14, 2018

  • 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 - got rid of records. I am using maps instead.
  • of course - added html.clj - default implementation of reporter.
  • modified README.
    Still lot of changes, but IMHO this is a minimum to allow reporting.

@tolitius
Copy link
Owner

nice, thanks for the work!

I have a couple of comments for small changes / improvements:

doc images

  • images "screen-1.png" and "screen-2.png" should have meaningful names: i.e. "sample-report.png" and "issue-details.png"
  • images should live under "doc/img": here is an example.

form files

  • "form-*" files should go to "tmpdir": example to support multiple OSs

sample report.html

  • a sample "report.html" file should live under "/examples"
  • there are both "forward slashes" and "backward slashes" in the "File" column of the report, is that easy to fix? (not urgent, but inconsistent): i.e. I see both: tolitius\boot\helper.clj and test/with_eastwood.clj

report

  • should have a datetime in the report filename: i.e. boot-check-report.2018-02-14T17:49:08.html
  • need to take a path/name for the report. This might be for the future PR, but I would imagine people would want to specify where the report goes and what to name it

logging

  • "reporting to html..." / "writing report to boot fileset TEMP directory..." should be at the DEBUG level
  • "Writing report to current directory..." should have a file name with the full path so there is no confusion

But overall all looks really good and it is great to look at the issues with details in the browser.

@voytech
Copy link
Contributor Author

voytech commented Feb 14, 2018

Hi I will apply your sugestions :)

Regarding "form files" - I think this comes from eastwood or other linter - Actually I do not produce such files. All files produced by me are persisted in tmpdirs as You suggested :) I think that I commited this by mistake (those form files) or introduced some switch for specific code checker which were passed to responsible linter tool.

@voytech
Copy link
Contributor Author

voytech commented Feb 15, 2018

FYI.
Currently there are pull requests made to kibit and bikeshed to enable better reporting of issues so that boot-check can make use of it (As I have said - using kibit - we cannot report file name of issue occurence (next version of kibit would allow that), using bikeshed - we are not getting any structure containing aggregated issues)

  • kibit - my PR was recently merged (after next version is released - boot-check kibit will report file names for issues in html report.)
  • bikeshed - we are waiting for accepting PR.

@voytech
Copy link
Contributor Author

voytech commented Feb 15, 2018

@tolitius I have provided all changes You requested.
I have also added option "skip-time?" - If someone has opened report in the browser and then runs boot-check again, he cannot simply reload report and needs to open another file (because date has changed). This option allows to skip time factor in the report filename so that report already opened in the browser can be easily reloaded without need for opening another tab.

@tolitius tolitius merged commit 8f3cc16 into tolitius:master Feb 15, 2018
@tolitius
Copy link
Owner

great, thanks for all the work

@voytech
Copy link
Contributor Author

voytech commented Feb 15, 2018

@tolitius Thanks, working with clojure is pleasure :)

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

Successfully merging this pull request may close these issues.

2 participants