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

Remove MP quickstart GreetApplication#getClasses method for auto-discovery #1395

Merged
merged 3 commits into from
Feb 20, 2020
Merged

Remove MP quickstart GreetApplication#getClasses method for auto-discovery #1395

merged 3 commits into from
Feb 20, 2020

Conversation

tjquinno
Copy link
Member

Addresses #1380

Removing the getClasses method from an app class allows auto-discovery to find resources automatically. This change showcases this feature in the quickstart (and stand-alone quickstart) MP apps.

@tjquinno tjquinno changed the title Comment out MP quickstart GreetApplication#getClasses method for auto-discovery Remove MP quickstart GreetApplication#getClasses method for auto-discovery Feb 14, 2020
@tomas-langer
Copy link
Member

I don't think this change is good.

  1. Application class is present to show how to list resources that belong to the app
  2. When you remove getClasses, the Application class does not serve any purpose in this example, so you could just remove it

I liked the way the quickstart was explicit - you create an app that has resources (and you can add an additional app, that has different resources).
Now you have a set of classes that "automagically" end up in the application you define by that class? I know this is the way people from app servers expect this to work, but it is not nice (IMHO).

Please let's talk about it before merging.

@spericas
Copy link
Member

@tomas-langer Adding multiple apps is not the norm. Developers with experience in JAX-RS would not normally do that. Now adding libraries with resources and expecting those to be part of your app is more common, in fact this is how this whole discussion started, from a developer question. Other MP implementations seem to favor scanning as well.

@tjquinno
Copy link
Member Author

tjquinno commented Feb 18, 2020

One concrete use case that came up in email is the MP extension openapi-ui. For this to work if getClasses is present, the developer has to modify that method after inspecting the source code for openapi-ui to find out what the service classes are.

I do not think we do want to force developers to do dive into the internals of external JARs to make them work with our example code.

@tjquinno
Copy link
Member Author

In light of this discussion, I propose to remove the GreetApplication class from the MP quickstart and MP stand-alone quickstart.

@tomas-langer Are you OK with this approach?

@tjquinno
Copy link
Member Author

Tomas reports he is OK with removing the application class.

Copy link
Member

@ljnelson ljnelson left a comment

Choose a reason for hiding this comment

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

LGTM

@tjquinno tjquinno merged commit bd3a849 into helidon-io:master Feb 20, 2020
@tjquinno tjquinno deleted the use-autodiscover-in-quickstart branch March 20, 2020 18:18
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.

None yet

4 participants