lastnpe / eclipse-external-annotations-m2e-plugin

M2E extension to setup Eclipse external annotations from pom.xml
http://lastnpe.org
Eclipse Public License 1.0
23 stars 11 forks source link

support maven properties for different classpath entries and paths #26

Closed maggu2810 closed 6 years ago

maggu2810 commented 6 years ago

The changes are split into two commits.

The first commit only changes some coding format. Mainly it adds final keyword where possible. If you agree that it doesn't make anything more bad, it would be great if we could use it.

The second commit contains the real code changes.

If you review the changes you should perhaps look at the second commit only.

Fixes: https://github.com/lastnpe/eclipse-external-annotations-m2e-plugin/issues/24

vorburger commented 6 years ago

@sylvainlaurent is this something you would be interested in to look over?

@maggu2810 I'll review and hopefully merge it if @sylvainlaurent doesn't get to it. Thank You for the 2 separate commits. If you had even raised it into 2 PRs, then I would have merged the only formatting one ASAP, and taken more time to review the other one... :smile:

_And on the specific topic of adding final more FYI and for fun, the following is NOT a review comment for this PR, I really don't care about it in this context and am happy to merge this, BUT... :smilecat: ... personally I actually sit on the OTHER side of that particular debate! Using a quality control check such as http://errorprone.info/bugpattern/Var I prefer to assume final is default... and remove final, as started to do e.g. here https://git.opendaylight.org/gerrit/#/c/65151/. Totally off topic and just as an FYI.

maggu2810 commented 6 years ago

Now there is only one commit remaining. The plugin should be backward compatible and old setups should work as previously.

sylvainlaurent commented 6 years ago

I'll have a look, hopefully in a few hours

maggu2810 commented 6 years ago
  • while we are adding m2e.eea.annotationpath.*properties, why not rename the global property with the same prefix? This would break existing users, unless we support both the old and new property.

See https://github.com/lastnpe/eclipse-external-annotations-m2e-plugin/issues/24#issuecomment-333224717

For backwards compatibility for any other existing users, IMHO it's probably best not to change m2e.jdt.annotationpath, but have new large ones, something like say m2e.maven.annotationpath and m2e.jre.annotationpath.

why do we make the different levels mutually exclusive?

This PR adds a new feature, it does not change the logic that has already been present. IMHO changing the logic should not done in a PR that adds "support maven properties for different classpath entries and paths".

maggu2810 commented 6 years ago

@vorburger ping

vorburger commented 6 years ago

@solf given your comments in https://github.com/lastnpe/eclipse-external-annotations-m2e-plugin/issues/24 would you like to confirm or even test this PR to confirm that it would address "your" https://github.com/lastnpe/eclipse-external-annotations-m2e-plugin/issues/24 and that could be closed when this is merged?

solf commented 6 years ago

@vorburger only partially -- it works but there's still no way to have a global default.

Global default can be achieved by e.g. modifying ClasspathConfigurator.getSingleProjectWideAnnotationPath like this:

        String property = properties.getProperty(propertyName);
        if (property == null) {
                property = System.getProperty(propertyName);
        }

With this in place it is possible to specify in eclipse.ini something like this: -Dm2e.eea.annotationpath.maven=/ext-eclipse-annotations/maven

If someone's willing to explain to me how to create pull request for this, I would create one. I already spent a bunch of time trying to figure out how to get @maggu2810 changes locally via git -- and I failed completely. I only managed to download zip copy of repository with his changes so that I could test it (e.g. this didn't work for me: https://docs.gitlab.com/ee/user/project/merge_requests/#checkout-locally-by-modifying-git-config-for-a-given-repository -- or at least I couldn't figure out what the proper checkout command is).

sylvainlaurent commented 6 years ago

No reserve from me

vorburger commented 6 years ago

@solf I've commented over in https://github.com/lastnpe/eclipse-external-annotations-m2e-plugin/issues/24, but will merge this change contributed by @maggu2810 now as @sylvainlaurent has no objections.