Thanks for the review. I will apply all suggestions from this email,
your comments on github and Luke's emails and come back to you afterwards.
On 01/10/2013 08:07 AM, Adam Murdoch wrote:
On 10/01/2013, at 9:40 AM, Marcin Erdmann wrote:
Looks like I've finally managed to more less finish the first
implementation step ('Add a build dashboard report') of the reporting
design document. It might still be rough around the edges so I'm
looking forward to your feedback to be able to polish it a bit more.
Code is available at:
https://github.com/erdi/gradle/tree/reporting-improvements
The code looks really good.
Following are the questions/concerns/deficiencies I have/see:
- There is only a little bit of documentation in there, can you
please tell me what are the expectations in this area, preferably
with some code examples? Which parts definitely have to be javadoced?
How do I document DSLs?
Generally, I would expect something like this:
- Some kind of mention of the plugin in the user guide - perhaps a
reporting chapter, or perhaps a plugin specific chapter. It doesn't
need to be very detailed initially and can grow over time as we add
more stuff. I wouldn't expect more than a couple of paragraphs initially.
- A sample, referenced by the user guide.
- Javadocs on the public types and methods (running `gradle
docs:check` will complain if you've missed something).
- The public types referenced in the dsl guide (have a look in
subprojects/docs/src/docs/dsl).
- Apart from tests everything is written in Java as I don't know
where in production code I'm allowed or even should use Groovy. Some
code, the plugin class in particular, would be much simpler if I was
allowed to write it in Groovy. I'm happy to rewrite anything you
think should be coded in Groovy.
It's fine to use groovy for the plugin and task implementations. I
wouldn't use it for any of the other production code. We would always
implement the unit and integration tests using spock (+ groovy, of
course).
- The dashboard is dead simple and might use some better styling, but
I'm not a designer so I would appreciate really precise directions in
that matter. It's currently using base-style.css and the header looks
similar to that in test results. Please also check if you are happy
with the wording used.
- I believe test coverage is decent, but if you see anything missing
please let me know. Report generating POJO is unit tested and task +
plugin are integration tested.
Tests look good to me.
- I added displayName property to Report, currently the
implementation for task generated reports holds values like 'Report
generated by task ':subproject:codenarcMain' (html)', is that ok?
- In BuildDashboardGenerator#generate() I'm rethrowing a possible
IOException by wrapping it in a RuntimeException, is this the right
approach?
I've added a comment to the commit.
- As per Luke's suggestion I'm using JATL to generate report HTML.
- I'm using Jerry to parse and search through the report HTML in
tests, Jerry allows jQuery like selectors on HTML in Java
(http://jodd.org/doc/jerry/index.html).
- Report is generated under 'buildDashboard/index.html' relatively to
baseDir of ReportingExtension.
This is all good.
--
Adam Murdoch
Gradle Co-founder
http://www.gradle.org
VP of Engineering, Gradleware Inc. - Gradle Training, Support, Consulting
http://www.gradleware.com
