Monday, September 26, 2005

Martin writes back and the plot thickens...

Right after I sent in the patch, I got this back from Martin.

- - - - - - - - - -
Thanks a lot John, the patch looks good. I still think strongly that we
shouldn't introduce a new variable, as this just adds completixy for the user.
In getTypeContent,
context.setVariable(CodeTemplateContextType.TYPENAME,
Signature.getQualifier(cu.getElementName()));
only works for top level types. I think an extra parameter is needed for the
enclosing type name.
I was wondering, would you also be happy having simply a type _body_ template?
That would by symetrical to the method body template and save us the new
variable for the header declaration. By deault such a variable would be empty.
Sorry that I realized this quite late. But if there arent strong reasons against
this I would favour such a template.

- - - - - - - - - -

..And I thought I was done. The part about the "body" template was the biggest challenge. Why had we worked our way to the design of a type template with a "header" variable that the user could put anywhere in the class? It seemed like such a great idea at the time when the first design I thought of was to embed the class definition straight into the new file template. Was Martin just trying to make things as similar as possible to the old way eclipse used to work? What about the incredible power and flexibility users will get from having control over where the header and brackets go in the class definitions?

Well, now that somebody challenged it, I couldn't really think of a good use case that couldn't be solved just as well as Martin suggested. If you want to put some standard comments or embedded metadata in comments before every class or after, why not put it in the new file template? The only thing you couldn't do is put something between the class header and the opening bracket. But why would anyone want to do that?

Pondering these issues, I explained the situation to some of the developers at work, and they totally sided with Martin. "That's the way I would have done it..." So I'm starting to feel like a real idiot for ever thinking the way I just coded this up was a good idea. Of course nurturing my fragile ego has never been my co-workers' strong suit.

On top of all this, the failure cases with all the solutions I thought of up until now always bothered me. What if users forgot to put in the ${type_header} variable or forgot one of the brackets? Martin's idea solves that problem by having eclipse generate the header and brackets, and still giving users a template variable for the contents they can fill in. So I caved again, and wrote back...

- - - - - - - - - -

Martin,
I thought about your suggestion to cut down the typecontent variable to just
the typebodycontent and have eclipse generate the rest automatically. I can
easily make this change. Your comment about it being "quite late" - do you
mean I need to make this change quickly to make a deadline, or just that I
have done the work already?
I can think of two reasons to keep it the way it is, but neither is very
strong. The first reason is that at some point, somebody may want to take
advantage of the flexibility to put a special comment or something around the
header. I can't think of a good use case for this, though, so the simplicity
of your suggestion wins out, in my mind.
The second reason is that it may be *less* intuitive, rather than more to edit
a blank template rather than one with a ${type_header} variable and two
brackets - at least that looks like a class declaration rather than just a
blank space - what do I put here? I don't think this is a very strong argument
either, as it may just be more intutive to me because I wrote it that way.
Also, the typebodycontent cuts out the failure cases in the typecontent of
leaving out the header or one or the other of the brackets - I discussed that
problem earlier in the thread.
OK - two things.
First, you mention TYPENAME only works for top-level types. Do you want me to
detect top-level or not and switch which variable I include based on that? Can
I get to the type name in any way for an inner type?
Second, I think the default template will have to be one newline character
rather than blank to match the current behavior, but not hard-code the newline
so that the user can never get rid of it. I'm assuming this is a small issue,
but the default template won't exactly be blank.
Thanks,
- John
- - - - - - - - - -

The next day I had the new patch ready.

- - - - - - - - - -

Here is the new version with just a blank type body as the new template.
The variables are the same as the first patch (just the type name added to all
the default variables). Type header is now gone and the default template is one
blank line as mentioned in the comment above.

0 Comments:

Post a Comment

<< Home