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

Use reflection to load Checks base on checkstyle_packages.xml #3607

Closed
romani opened this Issue Dec 2, 2016 · 9 comments

Comments

Projects
None yet
3 participants
@romani
Member

romani commented Dec 2, 2016

base on #3184

Consider to use reflection.
Use checkstyle_packages.xml and our naming notation that Check class should be "*Check".
How to get all classes from package - http://stackoverflow.com/a/520339

But problem will still be with extensions(custom Checks in custom jars), for example sevntu extension.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Apr 3, 2017

Contributor

@romani
Are you sure that we need additional library (Reflections) for this?
We can scan the package and get all file names which end with Check and then instantiate the classes based on the fully qualified class names.

Contributor

MEZk commented Apr 3, 2017

@romani
Are you sure that we need additional library (Reflections) for this?
We can scan the package and get all file names which end with Check and then instantiate the classes based on the fully qualified class names.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Apr 4, 2017

Member

our naming notation that Check class should be "*Check".

We dropped this convention in test's CheckUtil because it missed out on our own checks that end with Holder instead of Check.
I think checking the class' inheritance is enough, who cares what it is named especially for 3rd party checks.

We can scan the package

@MEZk I don't think we can scan packages without an extra library as you have to look in the JARs that are on the classpath. It is not naturally part of the Java Reflection API. Guava may support it like the PR states, but we are trying to rely on it less.

I do question why we need this as we can already load 3rd party classes and our own just fine.
CheckUtil picks up everything fine for our tests.
Why do we need in-depth production runtime scanning?

I assume this issue wanted to just look into the idea and discuss, not go straight to implementation.

Member

rnveach commented Apr 4, 2017

our naming notation that Check class should be "*Check".

We dropped this convention in test's CheckUtil because it missed out on our own checks that end with Holder instead of Check.
I think checking the class' inheritance is enough, who cares what it is named especially for 3rd party checks.

We can scan the package

@MEZk I don't think we can scan packages without an extra library as you have to look in the JARs that are on the classpath. It is not naturally part of the Java Reflection API. Guava may support it like the PR states, but we are trying to rely on it less.

I do question why we need this as we can already load 3rd party classes and our own just fine.
CheckUtil picks up everything fine for our tests.
Why do we need in-depth production runtime scanning?

I assume this issue wanted to just look into the idea and discuss, not go straight to implementation.

Luolc added a commit to Luolc/checkstyle that referenced this issue Apr 4, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Apr 4, 2017

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Apr 4, 2017

Contributor

I do question why we need this as we can already load 3rd party classes and our own just fine.
CheckUtil picks up everything fine for our tests.

Agree. Why do we need the extra dependency on Reflections?
We can continue using ClassPath from Guava.

Guava may support it like the PR states, but we are trying to rely on it less.

I still think that it is better to use the dependency (Guava) that we already use than add the new one.

@romani
Please, share your ideas.

Contributor

MEZk commented Apr 4, 2017

I do question why we need this as we can already load 3rd party classes and our own just fine.
CheckUtil picks up everything fine for our tests.

Agree. Why do we need the extra dependency on Reflections?
We can continue using ClassPath from Guava.

Guava may support it like the PR states, but we are trying to rely on it less.

I still think that it is better to use the dependency (Guava) that we already use than add the new one.

@romani
Please, share your ideas.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 8, 2017

Member

If we can do scan without extra dependencies it is good.

Reuse guava is ok, as we can not drop it as we use their fancy collections.

I do not know the best solution for this problem, we can look at PRs for this.

Member

romani commented Apr 8, 2017

If we can do scan without extra dependencies it is good.

Reuse guava is ok, as we can not drop it as we use their fancy collections.

I do not know the best solution for this problem, we can look at PRs for this.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Apr 9, 2017

Member

should we finish our discussion in issue first to make expected design decision?

@romani I don't think I have anything more to add to issue.
If we are able to find all our checks without issue, I don't see why we need this new dependency.
If we need this to be able to find 3rd party checks, please give us an example where current implementation fails in this regard or new dependency does it better.

Member

rnveach commented Apr 9, 2017

should we finish our discussion in issue first to make expected design decision?

@romani I don't think I have anything more to add to issue.
If we are able to find all our checks without issue, I don't see why we need this new dependency.
If we need this to be able to find 3rd party checks, please give us an example where current implementation fails in this regard or new dependency does it better.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Apr 10, 2017

Contributor

@rnveach

If we are able to find all our checks without issue, I don't see why we need this new dependency.

Completely agree. I'm against the new dependency. New dependency will bring new problems. There always be a desire to use it once again. As I sad at #3607, we do not introduce new checks very frequently, so we are able to add the checks into the map.

Contributor

MEZk commented Apr 10, 2017

@rnveach

If we are able to find all our checks without issue, I don't see why we need this new dependency.

Completely agree. I'm against the new dependency. New dependency will bring new problems. There always be a desire to use it once again. As I sad at #3607, we do not introduce new checks very frequently, so we are able to add the checks into the map.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Apr 15, 2017

Contributor

@romani
Can you share your opinion based on #3607 (comment) and #3607 (comment) to allow us to continue #4152 ?

Contributor

MEZk commented Apr 15, 2017

@romani
Can you share your opinion based on #3607 (comment) and #3607 (comment) to allow us to continue #4152 ?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 18, 2017

Member

lets consider solution without any extra dependency.
No changes to hardcoded list of Check, it should stay in code. We need reflection like search only for thirdparty libraries (like sevntu-checkstyle)

Member

romani commented Apr 18, 2017

lets consider solution without any extra dependency.
No changes to hardcoded list of Check, it should stay in code. We need reflection like search only for thirdparty libraries (like sevntu-checkstyle)

Luolc added a commit to Luolc/checkstyle that referenced this issue Apr 25, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Apr 25, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Apr 25, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Apr 26, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Apr 26, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Apr 26, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Apr 27, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Apr 30, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Apr 30, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue May 6, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue May 7, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue May 7, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue May 18, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue May 18, 2017

romani added a commit that referenced this issue May 18, 2017

@romani romani added this to the 7.8 milestone May 18, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani May 18, 2017

Member

fix is merged.

Member

romani commented May 18, 2017

fix is merged.

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