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

WiP: refactor SubstitutionVisitor. Unify conversion of template parameters #1350

Closed

Conversation

pvojtechovsky
Copy link
Collaborator

@pvojtechovsky pvojtechovsky commented May 31, 2017

Motivation: clean and simplify code of SubstitutionVisitor, before next refactoring, which will support decoupled template model and 'template parameters', which allows even more powerful templates.

Following changes were done on SubstitutionVisitor in this PR:

  • the list of parameter names, was replaced by Map of parameter names mapped to parameter values - is faster and is first step on the way to decouple template model and template parameters.
  • the conversion of template parameter values was unified and extracted into new Parameters#getParameterValueAsXXX methods - it is now cleaner what it does, corner cases are handled consistently, errors are reported. duplicate code was removed.
  • few bugs were fixed

TODO: refactor array access (the places which still use Parameters#getValue(...))

@pvojtechovsky pvojtechovsky force-pushed the refSubstitutionVisitor branch 2 times, most recently from 14900c1 to 7a5d624 Compare June 1, 2017 20:43
@pvojtechovsky pvojtechovsky changed the title WiP: refactor SubstitutionVisitor. Unify conversion of template parameters review3: refactor SubstitutionVisitor. Unify conversion of template parameters Jun 1, 2017
@pvojtechovsky pvojtechovsky force-pushed the refSubstitutionVisitor branch from 7a5d624 to 8eb0bea Compare June 2, 2017 18:27

@Test
public void testStatementTemplateRootSubstitution() throws Exception {
//contract: the template engine supports substitution of root element
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean by "root element"?

in new SubstituteRootTemplate(param).apply(resultKlass); do you mean param or resultKlass or something else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Root element is in this case the body of the statement method.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does that mean that the body of the statement method is never substituted?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, if I remember well (90%) it was not substituted. Or it failed with exception (10%)

Factory factory = spoon.getFactory();

CtClass<?> templateClass = factory.Class().get(SubstituteRootTemplate.class);
CtBlock<Void> param = (CtBlock) templateClass.getMethod("sampleBlock").getBody();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to templateParam? (param suggests method parameter)


@Override
public void statement() throws Throwable {
block.S();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean by "root element"?

This is root element of testStatementTemplateRootSubstitution()

@pvojtechovsky pvojtechovsky changed the title review3: refactor SubstitutionVisitor. Unify conversion of template parameters WiP: refactor SubstitutionVisitor. Unify conversion of template parameters Jun 6, 2017
@pvojtechovsky
Copy link
Collaborator Author

@monperrus
The new test testStatementTemplateRootSubstitution() fails on HEAD. The PR #1354 fixes it, but yesterday I found that it is only partial fix, because client can replace any element by
A) another element
B) no element
C) more then one another element - and this is the case which is not covered yet

The solution of C) will simplify SubstitutionVisitor (forEach and CtExecutable#parameters replacing, ...), but it causes deep refactoring too ... so I am actually checking how it will look like and then we can have a look how to split it into smaller PRs...

@pvojtechovsky
Copy link
Collaborator Author

this PR is contained in #1359. Closing this one.

@pvojtechovsky pvojtechovsky deleted the refSubstitutionVisitor branch June 12, 2017 19:13
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