review: feature: add "Generated by ..." comments automatically

[pvojtechovsky]

SubstitutionVisitor adds "Generated by ..." java doc comment automatically now.

For example:

    /**
     * Generated by a.b.c..RequestBuilderTemplate(RequestBuilderTemplate.java:25)
     */
    public CreateAnswerWithTargetBuilder(ServiceAPI serviceWrapper) {
        this.serviceWrapper = serviceWrapper;
        request = new CreateAnswerWithTargetReq();
    }

TODO: make it configurable whether it is added or not

[spoon-bot]

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.9.0-20170714.224607-7

New API: fr.inria.gforge.spoon:spoon-core:jar:5.9.0-SNAPSHOT

Detected changes: 2.

Change 1

Name	Element
Old	none
New	method boolean spoon.compiler.Environment::isAddGeneratedBy()
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking,

Change 2

Name	Element
Old	none
New	method void spoon.compiler.Environment::setAddGeneratedBy(boolean)
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking,

[monperrus]

I like the idea of this PR.

But I don't think that having a lot a boolean, somewhat cryptic, options in Environment is good. IMHO, this is already a problem we have and we should not worsen the situation.

Coming back to this PR here are possible solutions:

• not having an option
• having the option as a (public?) field in SubstitutionVisitor

WDYT?

[pvojtechovsky]

criptic configuration of spoon

I agree. I personally cannot configure spoon launcher easily. I always copy the code from somewhere...

public field in SubstitutionVisitor

I agree. This is good step. But it is not enough, because clients of Templates has no easy way how to set this field. So we need some support here too.

In future I would like to use TemplateBuilder, with appropriate related setter

[pvojtechovsky]

I removed Environment setter. I added SubstitutionVisitor#addGeneratedBy(boolean) and AbstractTemplate#addGeneratedBy(boolen)

OK now?

[monperrus]

Thanks a lot, that's much better and clearer.

One more problem is the duplication of the boolean setting in SubstitutionVisitor and AbstractTemplate: one does not really know who is responsible for this.

Here is a tentative solution: instead of

new SuperTemplate().addGeneratedBy(true).apply(superc);

I'd propose something like this:

// only SubstitutionVisitor is responsible of its configuration
template = new SuperTemplate();
template.getSubstitutionVisitor().addGeneratedBy(true)
template.apply(superc);

it's more verbose (but it's a special advanced case), but it follows the Single Responsibility Principle.

WDYT?

[pvojtechovsky]

Your suggestion is not compatible with all spoon.template.Substitution static methods. All of them expects that Template exists first and that SubstitutionVisitor is created many times internally in Substitution#substitute(...)

There is also problem that Template must be configured before SubstitutionVisitor is created for the template, because the template parameters are collected at SubstitutionVisitor creation time.

So I suggest to keep it as it is. It is may be not nice, but better solution would need more refactoring and would cause changes in API

[monperrus]

better solution would need more refactoring and would cause changes in API

I agree.

thanks Pavel for this nice feature.