Tapestry Training -- From The Source

Let me help you get your team up to speed in Tapestry ... fast. Visit howardlewisship.com for details on training, mentoring and support!

Thursday, May 23, 2013

Once more ... feedback please!

You've probably heard about "Not Invented Here" syndrome: the drive among developers to create something of their own, rather than just use an off-the-shelf library or component. It's almost universally painted as a bad thing, a sign of immaturity, or even arrogance.

But there's a flip side to this: every bit of code ever written contains within it tradeoffs: speed versus maintainability is a common tradeoff that everyone has seen. Perhaps the code is insufficiently flexible in the face of real-world requirements, but is really well tested for what it does cover. These choices reflect the developer's principles applied to the code. In fact, it is rare for it to be an easy give-and-take between two simple goals; more likely, there's lots of conflicting goals in the code, in the requirements, and in the developer's head. "Not Invented Here" can also mean "Not Reflecting My Principles".

Tapestry has it own set of guiding principals: Simplicity, Consistency, Efficiency, and Feedback ... and as a reusable framework, Feedback is very important. Feedback may be the most important principle when things go wrong. A framework that obscures problems, through bad feedback, is a framework that shouldn't be used.

Which brings us back to "Not Invented Here". Only in an impossibly perfect world would there be some ideal blob of code out there, ready to be reused, with zero impedance mismatch issues. In fact, when you bring in other people's code, you are forced to mesh your goals and principals with theirs. In my case, I'm adding support to Tapestry for converting Less files to CSS, using WRO4J (Web Resource Optimizer for Java). They've been working on WRO4J for several years, it makes sense to reuse their code, and it would be arrogant to think I could whip something better together under any kind of time constraints.

In fact, it's actually been pretty smooth sailing ... until we tripped across a violation of Tapestry's Feedback principle. As soon as I tested an error case, where the Less source file was not valid I hit bad feedback. Can you spot what's wrong with this exception report?

Oops! Looks like someone didn't get the memo about the importance of toString(). See, that Feedback principal is important to me, but for the majority of developers, useful feedback is too often an afterthought. I don't want to single out WRO4J here ... I'm pretty disdainful about feedback in nearly all software: open source or proprietary.

So what are our options here?

  • Write our own wrappers around the Less Processor, and throw out WRO4J
  • Beg the WRO4J guys to implement a real toString(), and wait for the next release
  • Fork WRO4J in the short term, and hope they'll take a patch in the long term
  • Patch around this reporting problem

Obviously, we should find a way to patch the reporting problem; we don't want to throw out the baby with the bath water. Fortunately, Tapestry provides the necessary hooks to override how it presents objects inside the exception report; it's all about providing a mapping from a Java type to a matching implementation of ObjectRenderer. Because of Tapestry's IoC container, this is actually quite straight forward:

And with those changes, the exception is presented quite differently:

Well, those are actually the raw ANTLR parser errors, but at least that's enough to help you find location of the problem ... whereas, with the bad feedback, you would only know that there was an issue somewhere in your Less source file.

4 comments:

Alex said...

I would be interested in the test-case which causes the bad beedback when using Less4jProcessor.

As far as I know, the friendly error reporting feature has been implemented since wro4j-1.6.3 (it is logging all Less4jException's found during processing). Could you provide more details regarding the version being used and the problematic less code?

When this kind of problems are reported, they are usually fixed very quick.

Thank you for the feedback,
Alex

Howard Lewis Ship said...

@Alex

Yes, more details are found in the console, but for a Tapestry developer, the console is a secondary source of information; they expect proper feedback in the exception report, and the lack of a toString() on the exception class prevented that.

In fact, I'd love to see a more uniform approach to error reporting across all of WRO4J; but it would take having an additional parameter on the ResourcePreProcessor interface, something I might call an ErrorSink, that is invoked when a processor sees an error.

Alex said...

I'm really interested in your suggestions. Referencing some examples would be very useful.

I would prefer not changing the ResourcePreProcessor interface, since it would break the API.

Error reporting is a responsibility of each processor, it is not very easy to achieve uniform approach since each processing deals with different type of exceptions and problems. Nevertheless, the client code can extend the processor or decorate it in order to catch and log exceptions differently than it is performed by default.

The reason why I'm asking for the sampl less code, is to add it to the suite of existing tests. I already have few tests for exception handling use-case, but it might not be enough. Could you point the repo containing this test-case?

Thanks again,
Alex

Howard Lewis Ship said...

True, not all of your processors are about line-oriented source files such as CSS and JS so that's an issue in terms of exception reporting.

In terms of Less; I just removed a few close braces from a Less file to see how such errors would be reported; that's when I discovered the missing toString() in the Tapestry exception report.

For me, the ultimate point of an exception is to report it to the user; in a T5 app, as part of the exception report page.