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.

Sign up for GitHub

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

Jump to bottom

review: refactor: manage sourceOutputDirectory in Environment #1770

Merged
merged 6 commits into from
Dec 8, 2017
Merged

review: refactor: manage sourceOutputDirectory in Environment #1770

merged 6 commits into from
Dec 8, 2017

Conversation

surli
Copy link
Collaborator

@surli surli commented Dec 4, 2017

This PR intends to fix issues bring by the multiple usage of SourceOutputDirectory

@INRIA INRIA deleted a comment from spoon-bot Dec 4, 2017
@INRIA INRIA deleted a comment from spoon-bot Dec 4, 2017
@surli surli changed the title refactor: manage sourceOutputDirectory in Environment review: refactor: manage sourceOutputDirectory in Environment Dec 4, 2017
tdurieux
tdurieux reviewed Dec 4, 2017
View reviewed changes
src/main/java/spoon/support/compiler/jdt/JDTBasedSpoonCompiler.java
@@ -279,12 +278,12 @@ public void addTemplateSources(List<SpoonResource> resources) {

@Override
public void setSourceOutputDirectory(File outputDirectory) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

put deprecated?

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 wondered: it's still usable to put the right value in the environment...

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The method is used in some test and maybe some clients are still using it to put the value, so it can still work except now it puts the value in the Environment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This PR will already break the API.
I would break it more to reduce the number of API, to simplify the usage of the API.
At least I would say in the doc that this API is just a different way to access the Environment

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 would break it more to reduce the number of API, to simplify the usage of the API.
At least I would say in the doc that this API is just a different way to access the Environment

I agree with you. I'll deprecate it then.

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 put the deprecated annotation in the interface.

@tdurieux
Copy link
Collaborator

tdurieux commented Dec 4, 2017

❤️

@monperrus
Copy link
Collaborator

OK for me. Merge?

@surli
Copy link
Collaborator Author

surli commented Dec 5, 2017

It's ready for me.

@surli
Copy link
Collaborator Author

surli commented Dec 6, 2017

@tdurieux @monperrus I made few changes to deprecate methods setOutputDirectory in SpoonModelBuilder and FileGenerator in favor of Environment#setOutputSourceDirectory.
However I think the API of JavaOutputProcessor is not really clear: it's a processor, except we never set its factory in Spoon, so here the Environment comes from the prettyprinter I pass in constructor, but it may came from the factory.

So maybe we should override the setFactory() to do nothing in JavaOutputProcessor and to only use the environment from the pretty printer to avoid inconsistency if two different environments are used in Factory and Prettyprinter.

@INRIA INRIA deleted a comment from spoon-bot Dec 6, 2017
surli
surli commented Dec 6, 2017
View reviewed changes
src/main/java/spoon/support/JavaOutputProcessor.java Outdated
@@ -96,7 +122,7 @@ public void init() {
}
}
try {
directory = directory.getCanonicalFile();
this.getEnvironment().setSourceOutputDirectory(directory.getCanonicalFile());
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'm not sure about this one: this should be done directly in the setter of Environment, no? WDYT?

tdurieux
tdurieux reviewed Dec 6, 2017
View reviewed changes
src/main/java/spoon/support/JavaOutputProcessor.java Outdated

List<File> printedFiles = new ArrayList<>();

public JavaOutputProcessor(PrettyPrinter printer) {
this.printer = printer;
Copy link
Collaborator

Choose a reason for hiding this comment

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

that does not seem nice :/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're talking about the line below?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No the constructor

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could you elaborate ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

why did you need to copy the env?
it is accessible via getFactory().getEnv().

@monperrus
Copy link
Collaborator

OK for me. @tdurieux do you see a problem?

@tdurieux
Copy link
Collaborator

tdurieux commented Dec 7, 2017

I have a general issue with the printing process, there is too much state.

@tdurieux
Copy link
Collaborator

tdurieux commented Dec 7, 2017

I don't like the creation of getEnvironment method in several classes, I am pretty sure it is not required

@monperrus
Copy link
Collaborator

I don't like the creation of getEnvironment method in several classes

I agree, if we can avoid it, that's better

monperrus
monperrus reviewed Dec 7, 2017
View reviewed changes
src/main/java/spoon/support/JavaOutputProcessor.java Outdated
@@ -44,19 +46,28 @@
*/
public class JavaOutputProcessor extends AbstractProcessor<CtNamedElement> implements FileGenerator<CtNamedElement> {
PrettyPrinter printer;

File directory;
private Environment environment;
Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't need some state here, we alread have getFactory().getEnvironment()

monperrus
monperrus reviewed Dec 7, 2017
View reviewed changes
src/main/java/spoon/support/JavaOutputProcessor.java Outdated

List<File> printedFiles = new ArrayList<>();

public JavaOutputProcessor(PrettyPrinter printer) {
this.printer = printer;
this.environment = printer.getEnvironment();
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can replace printer.getEnvironment() by getFactory().getEnvironment(). And then we don't need the new method in interface PrettyPrinter

@INRIA INRIA deleted a comment from spoon-bot Dec 8, 2017
@tdurieux
Copy link
Collaborator

tdurieux commented Dec 8, 2017

thanks!

@INRIA INRIA deleted a comment from spoon-bot Dec 8, 2017
@spoon-bot
Copy link
Collaborator

Detected changes by Revapi: 3.

Old API: fr.inria.gforge.spoon:spoon-core:jar:6.1.0-20171207.234709-36

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

Name Change 1
Old field JDTBasedSpoonCompiler.outputDirectory
New none
Code java.field.removed
Description Field removed from class.
Breaking binary: breaking,
Name Change 2
Old none
New method Environment::getSourceOutputDirectory()
Code java.method.addedToInterface
Description Method was added to an interface.
Breaking binary: non_breaking,
Name Change 3
Old none
New method Environment::setSourceOutputDirectory(File)
Code java.method.addedToInterface
Description Method was added to an interface.
Breaking binary: non_breaking,

@surli
Copy link
Collaborator Author

surli commented Dec 8, 2017

@tdurieux @monperrus so it's ready for me.

@monperrus monperrus merged commit a3a9650 into INRIA:master Dec 8, 2017
@monperrus
Copy link
Collaborator

thanks a lot!

@monperrus monperrus mentioned this pull request Dec 8, 2017
Merged
@monperrus
Copy link
Collaborator

seems we have a regression due to this one: https://ci.inria.fr/sos/job/juliac/423/consoleFull

@seintur
Copy link
Contributor

seintur commented Dec 8, 2017

I was using

spoon/src/main/java/spoon/support/JavaOutputProcessor.java

Line 64 in a3a9650

public JavaOutputProcessor(File outputDirectory, PrettyPrinter printer) {
that is now deprecated. I updated my project to remove the use of this now deprecated method. The problem is fixed.

@monperrus
Copy link
Collaborator

monperrus commented Dec 8, 2017 via email

@surli surli mentioned this pull request Dec 20, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tdurieux tdurieux tdurieux left review comments

@monperrus monperrus monperrus left review comments

Assignees
No one assigned
Labels
breaking_pr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@surli @tdurieux @monperrus @spoon-bot @seintur