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: spring annotations project creation. #3492

Conversation

aureamunoz
Copy link
Member

Related to issue #3437

@geoand
Copy link
Contributor

geoand commented Aug 13, 2019

@gsmet @ia3andy could you review this please since my internet access will be very sporadic over the next week or so?

Thanks

@ia3andy
Copy link
Contributor

ia3andy commented Aug 13, 2019

@geoand correct me if I am wrong, I see two potential problems:

  1. I am not sure but it seems that the result of the create may depend on the order in which the builder is called
  2. when I used it in the launcher, I added the extensions after creating the project, do you assume that it's supposed to be effective only at creation time?

@geoand
Copy link
Contributor

geoand commented Aug 13, 2019

I won't really be able to review for a few days, so up to you really to request changes if you think they are necessary 😊

Copy link
Contributor

@ia3andy ia3andy left a comment

Choose a reason for hiding this comment

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

@gsmet, what's your opinion? I am not totally sure, but I think it would make more sense to have this feature part of the AddExtensions mechanism. It would create the Spring file, even when adding the spring extension in an existing project.

gsmet
gsmet previously requested changes Aug 13, 2019
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, I added a few comments inline.

@gsmet
Copy link
Member

gsmet commented Aug 13, 2019

@ia3andy I don't know. I'm not very excited at the idea of creating new files when adding an extension to an existing project.

@ia3andy
Copy link
Contributor

ia3andy commented Aug 13, 2019

@gsmet I don't know either, but having different behavior (before and after) is as troubling to me.. maybe this should not be treated automatically with the extension but more as an option in the create command?

@gsmet
Copy link
Member

gsmet commented Aug 13, 2019

I don't think it's an issue. For me it's two different things:

  • you create a project, it comes from a template and you have a few examples set up
  • you have an existing project, you add an extension and you certainly don't want it to add random files to your project.

I don't think it's an issue.

@ia3andy
Copy link
Contributor

ia3andy commented Aug 13, 2019

@gsmet ok, so I guess this PR includes two features:

  • Being able to set the extensions directly in CreateProject
  • Being able to detect and add extra content depending on the set of extensions (in this case for Spring) in CreateProject

The order of call seems to still be an issue, I'll update my "Change Request"..

@geoand
Copy link
Contributor

geoand commented Aug 17, 2019

@aureamunoz you will also have to rebase onto the latest master since there are some conflicts

@ia3andy
Copy link
Contributor

ia3andy commented Aug 20, 2019

@aureamunoz in your implementation, the extensions in CreateProject are only used to detect Spring, you could also use them to write them in the pom.xml directly at project creation (instead of having to do it in another step). I guess that what @gsmet said to do in another PR, right?

@aureamunoz aureamunoz force-pushed the issue3437-spring-annotation-project-creation branch from 6bba81a to 1c58752 Compare August 20, 2019 11:14
@aureamunoz aureamunoz force-pushed the issue3437-spring-annotation-project-creation branch from 1c58752 to 798c10e Compare August 21, 2019 06:10
@geoand
Copy link
Contributor

geoand commented Aug 21, 2019

@gsmet @ia3andy unless you have any objections, I plan to merge this

@gsmet
Copy link
Member

gsmet commented Aug 22, 2019

@geoand feel free to do it, I trust your judgement on this one.

@geoand
Copy link
Contributor

geoand commented Aug 22, 2019

Thanks!
Merging with what we have, we can certainly improve in the future - I suppose @ia3andy will have more ideas on this part

@geoand geoand dismissed gsmet’s stale review August 22, 2019 09:09

changes have been made

@geoand
Copy link
Contributor

geoand commented Aug 22, 2019

Tested locally and things work as expected.

Thanks for the PR!

@geoand geoand merged commit f9ea1a8 into quarkusio:master Aug 22, 2019
@gsmet gsmet added this to the 0.22.0 milestone Aug 28, 2019
@geoand geoand modified the milestones: 0.23.0, 0.22.0 Sep 13, 2019
@geoand geoand added the area/spring Issues relating to the Spring integration label Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/spring Issues relating to the Spring integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants