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

feat: add a static method to getting started with one single line of code #1559

Merged
merged 3 commits into from
Oct 2, 2017

Conversation

monperrus
Copy link
Collaborator

No description provided.

@pvojtechovsky
Copy link
Collaborator

Nice!

It is true that Launcher is not intuitive and I am often fighting with initial configuration of Spoon too.

@@ -798,4 +800,12 @@ public CtModel getModel() {
return factory.getModel();
}

/** returns the AST of an inline class */
public static CtClass<?> parseClass(String code) {
Copy link
Collaborator

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 your method return: I think this method will be quickly used with code containing multiple types (enum + class for example or multiple classes).
Then I see two solutions:

  1. Either the method return a list of CtTypes (and then we may have to rename it)
  2. Either it checks that the model contains only one CtClass (and then we explictely do not accept interfaces or enum, and most importantly we give a comprehensive error for the user)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or even code can contain only an interface...
I would return a CtType.

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 idea of this PR is to have the easiest possible introduction to Spoon. Having CtClass means that users would have directly getConstructors etc. which they would naturally expect (which is not in CtType). CtType is a slight less natural, more abstract concept. Once newcomers understand this method, they would obviously move to the complete configurable Launcher and the rest of the metamodel (incl CtType)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm ok for keeping CtClass return but we should then check the type before casting, to help the user in case he did not pass a simple class.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I would create a new Launcher instead of adding a new method in a class that contains already to much stuff.

Copy link
Collaborator

Choose a reason for hiding this comment

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

new Launcher

may be yes! But only as a replacement/wrapper of old non intuitive Launcher. I do not think we should have many ways/launchers how to start processing. The one "well designed" launcher would be nice ... but I actually have no constructive suggestion how it should look like. I just have problems with the old Launcher.

Copy link
Collaborator

@surli surli Sep 28, 2017

Choose a reason for hiding this comment

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

Maybe something as a LauncherBuilder:

CtModel model = new LauncherBuilder().useAutoimport()
         .useNoclasspath()
         .addInputSource("./myDirectory/")
         .addInputSource("./anotherOne")
         .setOutputDir("./target")
         .build();

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes exactly! But someone should analyze all Launcher use cases and organize them into intuitive API. I am not the one who knows use cases of Launcher well enough. Let's make an issue where we can build such a concept.

@tdurieux
Copy link
Collaborator

It is true that Launcher is not intuitive and I am often fighting with initial configuration of Spoon too.

Indeed, we need to improve the documentation of the Launcher.

@monperrus
Copy link
Collaborator Author

There is a confusion there.
The goal of this PR is to provide a one-liner intuitive introduction for newcomers.
Not to improve or redesign Launcher (which is a good idea, but for another PR)

@surli
Copy link
Collaborator

surli commented Sep 28, 2017

The goal of this PR is to provide a one-liner intuitive introduction for newcomers.

Yep, that's why we opened a new issue to discuss the design of a new launcher.

@surli surli merged commit 51c9a94 into INRIA:master Oct 2, 2017
@tdurieux
Copy link
Collaborator

tdurieux commented Oct 2, 2017

There is a confusion there.
The goal of this PR is to provide a one-liner intuitive introduction for newcomers.
Not to improve or redesign Launcher (which is a good idea, but for another PR)

For me the design of this PR is wrong. It will introduce more confusion in the API than solve issues.

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.

4 participants