Skip to content
This repository was archived by the owner on Nov 18, 2020. It is now read-only.

Conversation

@michaelcouck
Copy link
Contributor

Hi Mahmoud,

Just changed the resource loading in the annotation processor. When dynamically attaching the class loader for the base classes, and those that are added to the boot class path search, don't have a class loader at attach time, so the resource lookup fails. Strange case, I know.

Regards,
Michael

@coveralls
Copy link

coveralls commented Apr 10, 2018

Coverage Status

Coverage decreased (-0.5%) to 90.102% when pulling 7eaef20 on michaelcouck:master into 57ad505 on j-easy:master.

* @throws IOException when an error occurs during resource loading
*/
protected InputStream getResourceAsStream(final String resource) throws IOException {
InputStream getResourceAsStream(final String resource) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

I think this can still as protected. Why do we need to make it package private?

pom.xml Outdated

<groupId>org.jeasy</groupId>
<artifactId>easy-props</artifactId>
<artifactId>easy-props-ext</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this change. Why do we need to change the artifactId?

@@ -1,25 +1,25 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

Please leave these license header changes out of the PR. I use a template with the maven licence plugin to update these headers. So if there is an issue with the template, we need to change the template itself so that the changes are taken into account to all other files.

@fmbenhassine
Copy link
Member

Hi Michael,

Thank you for the PR! Strange case indeed, but the PR is valid 👍

I just added a couple of notes. If you can address them and then It'll be ready to merge.

Kr,
Mahmoud

@michaelcouck
Copy link
Contributor Author

Hi Mahmoud,

  1. No specific reason to change the privacy of the method, but more private is better?
  2. I don't have an private artifactory any more, so to over ride your artifact I had to change the name for the local repository
  3. IntelliJ did it not me :)

My choices were:

  1. Redefine your classes at runtime to avoid the null pointer - too much work
  2. Strip out your logic and do my own - also too much work
  3. Patch the annotation processor - simple!

Thanks!
Best regards,
Michael

@fmbenhassine
Copy link
Member

Hi,

  1. No specific reason to change the privacy of the method, but more private is better?

Of course! Ok keep this in the PR

I don't have an private artifactory any more, so to over ride your artifact I had to change the name for the local repository

Ok I see now, but please remove it from the PR.

IntelliJ did it not me :)

That's fine. I see from were it comes: Dangling javadoc comment warning. I will fix the license template file as explained and update all the license headers across the project. So please remove this change from the PR.

The rest looks good to me. Thank you upfront!

Kr
Mahmoud

@michaelcouck
Copy link
Contributor Author

Changed the

@michaelcouck
Copy link
Contributor Author

Done.

@fmbenhassine fmbenhassine reopened this Apr 13, 2018
@fmbenhassine fmbenhassine merged commit ab21b92 into j-easy:master Apr 13, 2018
@fmbenhassine fmbenhassine added this to the 2.1.0 milestone Apr 13, 2018
@fmbenhassine
Copy link
Member

fmbenhassine commented Apr 13, 2018

Thank you for the updates! I merged the PR. It will be part of v3.0 release.

Keep tuned.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants