Review: feat: be able to change the destination of each created file #1610

tdurieux · 2017-10-17T13:08:34Z

What do you think?

The idea of this is to be able to override DefaultOutputDestination and defines your output strategy in getOutputPath(File defaultOutputDirectory, Path packagePath, String filename)

monperrus · 2017-10-17T15:43:36Z

src/main/java/spoon/support/OutputDestination.java

+
+public interface OutputDestination {
+
+	Path getOutputPath(File defaultOutputDirectory, Path packagePath, String filename);


Path, Path?

monperrus · 2017-10-17T15:43:51Z

src/main/java/spoon/support/OutputDestination.java

+
+	Path getOutputPath(File defaultOutputDirectory, Path packagePath, String filename);
+
+	Path getOutputPath(CtPackage type, File defaultOutputDirectory,


additional arg at the end?

monperrus · 2017-10-17T15:47:15Z

src/main/java/spoon/Launcher.java

@@ -95,6 +95,8 @@
 	private List<String> processorTypes = new ArrayList<>();
 	private List<Processor<? extends CtElement>> processors = new ArrayList<>();

+	private OutputDestination outputDirectory = new DefaultOutputDestination();


no state plz

pvojtechovsky · 2017-10-17T18:12:04Z

I like that idea.
Note: Interface OutputDestination with 3 methods cannot be implemented using lambda. May be you can implement two of them as default methods directly in interface?

tdurieux · 2017-10-19T07:09:06Z

I think three methods is too much.
I propose to only have
getOutputPath(CtElement element, File defaultOutputDirectory, Path packagePath, String filename).

The users that will use this interface should be able to do an instanceof .

WDYT?

pvojtechovsky · 2017-10-19T07:17:11Z

What about method like this:

Path getOutputPath(CtPackage package, CtType type, File defaultOutputDirectory, Path packagePath, String filename)

the type might be sometime null

monperrus · 2017-11-10T09:41:40Z

ping?

tdurieux · 2017-11-10T10:12:28Z

I have an issue because the output destination is stored in two places in the:

ModelBuilder
FileGenerator

I think it is an issue, but I don't know which one is the best to keep

surli · 2017-12-01T16:01:04Z

I think it is an issue, but I don't know which one is the best to keep

I propose to put the output destination in Environment and to only store it there. Then other classes could get it through Environment. WDYT?

pvojtechovsky · 2017-12-02T18:34:01Z

to put the output destination in Environment

sounds good for me

monperrus · 2017-12-08T10:27:46Z

rebase now that we have #1770 ?

monperrus · 2017-12-18T15:37:03Z

travis fails? @tdurieux ?

tdurieux · 2017-12-18T15:41:54Z

no there is still a lot of work to do.
The current implementation is not nice, way to much state

…utput directory for each create file

tdurieux · 2017-12-20T09:36:47Z

I finished to clean up this PR.

But I'm not sure if the interface and the implementation of OutputDestination is good enough.
I made DefaultOutputDestination in the way that it is easy to overwrite.
Probably, the method that will be overwritten by the client is Path getDirectoryPath(CtModule module, CtPackage pack, CtType type) which returns the root path of the destination.

surli · 2017-12-20T12:12:31Z

src/main/java/spoon/compiler/Environment.java

+	/**
+	 * Set the output destination that handles where source files are written
+	 */
+	void setOutputDesination(OutputDestination outputDestination);


The name is a bit confusing with sourceOutputDirectory, and for me it looks like a strategy for the final location of the output. So WDYT about renaming it to something like setOutputStrategy or setOuputHandler?

surli · 2017-12-20T12:14:31Z

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

-		if (!packageDir.exists()) {
-			if (!packageDir.mkdirs()) {
+	private Path getElementPath(CtPackage type) {
+		return createFolders(getEnvironment().getOutputDestination()


Why don't you delegate the creation of the folder to the output destination strategy? We can imagine different implementation e.g. a strategy for not creating new directory, or for not overwritten existing files.

strategy for not creating a new directory, or for not overwritten existing files.

it is not exactly the same responsibility, what you ask is more something like, isToPrint, not whereToPrint

it is not exactly the same responsibility, what you ask is more something like, isToPrint, not whereToPrint

In fact I see the OutputDestinationStrategy as the main handler in Spoon to manage the pathes of the output. In that sense, its responsibility is indeed, not only to say where to print, but also to say if it's ok to print things at this specific location.
The idea behind it is that you only need to provide your own implementation of OutputDestinationStrategy whenever you need a specific behaviour related to path, beeing where, if or how.

I don't know how to communicate with the output processor. The use case is not clear to me.

Maybe just rename getOutputPath to createOutputPath, and inside this method you create the needed folders?..

surli · 2018-01-03T10:21:44Z

I changed some names so for me it's ready to merge @tdurieux @monperrus

monperrus · 2018-01-04T12:12:04Z

That's good. Could you add a test case for this new feature?

spoon-bot · 2018-01-11T09:41:21Z

Detected changes by Revapi: 2.

Old API: fr.inria.gforge.spoon:spoon-core:jar:6.2.0-20180110.234716-39

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

Name	Change 1
Old	none
New	method Environment::getOutputDestinationHandler()
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking

Name	Change 2
Old	none
New	method Environment::setOutputDestinationHandler(OutputDestinationHandler)
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking

spoon-bot · 2018-01-11T10:38:25Z

Detected changes by Revapi: 2.

Old API: fr.inria.gforge.spoon:spoon-core:jar:6.2.0-20180110.234716-39

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

Name	Change 1
Old	none
New	method Environment::getOutputDestinationHandler()
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking,

Name	Change 2
Old	none
New	method Environment::setOutputDestinationHandler(OutputDestinationHandler)
Code	java.method.addedToInterface
Description	Method was added to an interface.
Breaking	binary: non_breaking,

surli · 2018-01-11T10:38:50Z

test added @monperrus

monperrus · 2018-01-11T10:43:26Z

thanks a lot!

monperrus reviewed Oct 17, 2017

View reviewed changes

monperrus changed the title ~~wip: feat: be able to change the destination of each created file~~ WIP: feat: be able to change the destination of each created file Nov 10, 2017

tdurieux force-pushed the feat_output_destination branch from c9e7049 to fbd9940 Compare December 8, 2017 11:58

tdurieux added 9 commits December 20, 2017 10:30

feat(output): create an output destination to be able to change the o…

a117b36

…utput directory for each create file

fix compilation issue

aa621e7

wip

f9625a9

rebase issue

fcdb154

put the outputdestination is the environment

33f87a2

refactor default output desination

66bdc46

fix Module access

8f622cf

module only for java > 8

40bc2b4

fix current path

244a370

tdurieux force-pushed the feat_output_destination branch from 0ed5729 to 244a370 Compare December 20, 2017 09:31

tdurieux added 2 commits December 20, 2017 10:38

add javadoc

c436eee

fix module destination

54c6ffa

INRIA deleted a comment from spoon-bot Dec 20, 2017

tdurieux changed the title ~~WIP: feat: be able to change the destination of each created file~~ Review: feat: be able to change the destination of each created file Dec 20, 2017

INRIA deleted a comment from spoon-bot Dec 20, 2017

surli reviewed Dec 20, 2017

View reviewed changes

INRIA deleted a comment from spoon-bot Dec 20, 2017

surli added 2 commits January 3, 2018 10:34

Trigger CI

2105156

Rename variables and interface

0cfd0db

INRIA deleted a comment from spoon-bot Jan 3, 2018

Also use new output destination handler in CompilationUnitFactory

4ea3653

surli added 2 commits January 11, 2018 11:24

Add new tests and fix CUFactory

fc59906

Fix checkstyle

b6711e4

INRIA deleted a comment from spoon-bot Jan 11, 2018

monperrus merged commit 368343f into INRIA:master Jan 11, 2018

surli mentioned this pull request Mar 6, 2018

New release: Spoon 6.2.0 is here #1895

Closed


		public interface OutputDestination {

		Path getOutputPath(File defaultOutputDirectory, Path packagePath, String filename);


		Path getOutputPath(File defaultOutputDirectory, Path packagePath, String filename);

		Path getOutputPath(CtPackage type, File defaultOutputDirectory,

Uh oh!

Review: feat: be able to change the destination of each created file #1610

Review: feat: be able to change the destination of each created file #1610

Uh oh!

Conversation

tdurieux commented Oct 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pvojtechovsky commented Oct 17, 2017

Uh oh!

tdurieux commented Oct 19, 2017

Uh oh!

pvojtechovsky commented Oct 19, 2017

Uh oh!

monperrus commented Nov 10, 2017

Uh oh!

tdurieux commented Nov 10, 2017

Uh oh!

surli commented Dec 1, 2017

Uh oh!

pvojtechovsky commented Dec 2, 2017

Uh oh!

monperrus commented Dec 8, 2017

Uh oh!

monperrus commented Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tdurieux commented Dec 18, 2017

Uh oh!

tdurieux commented Dec 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surli commented Jan 3, 2018

Uh oh!

monperrus commented Jan 4, 2018

Uh oh!

spoon-bot commented Jan 11, 2018

Uh oh!

spoon-bot commented Jan 11, 2018

Uh oh!

surli commented Jan 11, 2018

Uh oh!

monperrus commented Jan 11, 2018

Uh oh!

Uh oh!

monperrus commented Dec 18, 2017 •

edited

Loading