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

New Feature #137: Implement withIoC={CDI, CDI_SINGLETON} options #141

Merged

Conversation

Musikolo
Copy link

Hi,

I've tried implementing the feature #137 I requested a few days ago. The code I've written looks like it works, but I honestly didn't manage to find a way to get the tests running. I've implemented a couple of new tests classes that are supposed to work, but again, since I couldn't make them run, I cannot confirm at 100%. I've been able to create another project and validate the generated mapper classes are good though.

I'm using Eclipse and Java 8. If you tell me how to run tests, I can try again. Nevertheless, you can see what you get, and if everything looks good to you, maybe you can merge the code straightway.

Please, let me know if you find anything that requires improvement.

Bes regards.

@slemesle
Copy link

Hi thanks for the pull request,

as you can see there is an Exception in fr.xebia.extras.selma.it.custom.mapper.CustomCdiSingletonMapperInMapsIT.

Selma is not able to find the generated Mapper class.
You used tabs instead of spaces for indentation which breaks change management as you changed all lines of the generator.

To be able to run the tests you can simply launch the build with maven. I personnaly use IntelliJ to run tests.

@Test
public void given_cdi_mapper_should_be_annotated_with_Inject_and_Singleton() {

final CustomCdiMapperInMaps mapper = Selma.getMapper(CustomCdiMapperInMaps.class);

Choose a reason for hiding this comment

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

Mapper here should be CustomCdiSingletonMapperInMaps not CustomCdiMapperInMaps.

/** Adds @Inject annotation to the generated mapper */
CDI,
/** Adds @Inject and @Singleton annotations to the generated mapper */
CDI_SINGLETON,

Choose a reason for hiding this comment

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

I am not sure why we should create 3 different values for CDI. Why don't you consider @Singleton as required for CDI.

/** Adds @Inject and @Singleton annotations to the generated mapper */
CDI_SINGLETON,
/** Adds @ApplicationScoped annotation to the generated mapper */
CDI_APPLICATION_SCOPED

Choose a reason for hiding this comment

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

Same as for singleton @ApplicationScoped can be the standard way.

@Musikolo
Copy link
Author

Musikolo commented Jan 19, 2017

Hi,

first of all, let me apologize for the unintended changes I pushed before. For some reason I still ignore, I had my Eclipse formatter configured to use tabs, as you pointed out. I have updated my formatter to use 4 spaces instead of tabs. All the modified classes should have now spaces instead of tabs.

In addition to that, after removing the tabs, I could validate that the tests were passing in my local this time. I don't know the reason why tabs caused such a huge impact in the code, but the important thing is that it's now solved.

I have also corrected the mapper being used in one of my test classes, as you pointed out. It clearly was a side effect of a too fast copy-and-paste maneuver.

Finally, the reason why I added @Inject and @Singleton annotation is because they belong to the JSR-330 CDI standard which allows injection in lightweight JavaSE containers such as Tomcat, Jetty, Undertow and many others. However @ApplicationScope belongs the EJB injection API which requires a fully J2EE complaint container such as JBoss, Oracle AS, IBM WebSphere, and others. Since the trend for most new applications is to use lightweight containers along with injection frameworks such as Spring, Guice, HK2 or even CDI, I think it makes more sense to define annotations the way I did. This said, I'm fine if you consider it should be done in a different way.

I hope everything is good know. However, please, do not hesitate to let me know if anything else is needed from my side.

Thank you!

@slemesle slemesle merged commit 95b8215 into publicissapient-france:master Jan 23, 2017
@slemesle
Copy link

Hi thanks for the great work,
I've just merged the PR.

It's all good for me :)

@slemesle slemesle added this to the 1.0 Final milestone Apr 30, 2017
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.

2 participants