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

alternative fix bug #921 Duplicity in package-info.java when templates are used #934

Merged

Conversation

pvojtechovsky
Copy link
Collaborator

@monperrus Hi Martin, here is the alternative PR for #932 as you suggested.

It is variant, which uses non abstract JDTBatchCompiler. FactoryCompiler and FileCompiler does not inherit from JDTBatchCompiler and they were renamed to FactoryCompilerConfig and FileCompilerConfig, because they are now used to configure the JDTBatchCompiler.

CompilationTest file and folder filtering tests are commented for now. There is no support for filtering by overwriting of createBatchCompiler. I suggest to discuss how to implement it later.

ExtendedStringLiteralTest was updated. It should be OK.

InputType was changed from enum to interface. The change is compiler compatible so source code of the clients does not have to be changed. Just recompiled. Implementation of the InputType can configure JDTBatchCompiler before the compilation is started.

	interface InputType {
		InputType FILES = FileCompilerConfig.INSTANCE;
		InputType CTTYPES = FactoryCompilerConfig.INSTANCE;

		void initializeCompiler(JDTBatchCompiler compiler);
	}

So we have standard defaults, but now they can extend and use different configurations if needed. I use it internally instead of InputType.UNIT, I am using

... createBatchCompiler(new FileCompilerConfig(sources));

Copy link
Collaborator

@monperrus monperrus left a comment

Choose a reason for hiding this comment

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

It is a good and creative solution. And again mostly backward compatible, congrats. See my two comments.

InputType FILES = FileCompilerConfig.INSTANCE;
InputType CTTYPES = FactoryCompilerConfig.INSTANCE;

void initializeCompiler(JDTBatchCompiler compiler);
Copy link
Collaborator

Choose a reason for hiding this comment

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

add javadoc:
/** responsible for setting the parameters of JDTBatchCompiler, must call setCompilationUnits() */

this.compilationUnits = compilationUnits;
}

public void setInputFiles(List<SpoonFile> files) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I propose that this method moves to FileCompilerConfig. And FileCompilerConfig would be responsible for calling setCompilationUnits().

@@ -154,7 +146,7 @@ public void testNewInstance() throws Exception {
}

}

/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is better to remove instead of commenting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have no problem to remove it, but I am not author. So @monperrus should agree that we will really get rid of these tests without any replacement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

since your architecture is mostly backward compatible, it seems that only minor changes will enable us to keep those tests passing. can you give it a try?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will give it a try, but I would prefer if you merge my changes first, because then I can create another PRs too, related to compiler this evening. Otherwise I can fix only test and cannot do more until it is merged. Because of conflicts ...

@pvojtechovsky pvojtechovsky force-pushed the fixDuplicitiesInPackageInfoOnMasterB branch from ce552dd to 47bd7d5 Compare November 7, 2016 12:19
@pvojtechovsky pvojtechovsky force-pushed the fixDuplicitiesInPackageInfoOnMasterB branch from 47bd7d5 to 6750a71 Compare November 7, 2016 13:07
@pvojtechovsky
Copy link
Collaborator Author

I have added comment and moved the method as suggested. I have also rebased to master

@monperrus monperrus merged commit 3b88ba8 into INRIA:master Nov 7, 2016
@pvojtechovsky pvojtechovsky deleted the fixDuplicitiesInPackageInfoOnMasterB branch November 16, 2016 18:25
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.

3 participants