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

internal code: terminolozy problems in ModuleReflectionUtils #4876

Closed
romani opened this Issue Aug 1, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@romani
Member

romani commented Aug 1, 2017

ModuleReflectionUtils.

There is method

public static boolean isCheckstyleCheck(Class<?> clazz) {
        return AbstractCheck.class.isAssignableFrom(clazz);
}

Check is term that is used for both FileSet modules and TreeWalker modules.
JavadocPackageCheck is also a Check but this method return false on it.
This is found during codereview at TreeWalkerTest.testWithCheckNotHavingTreeWalkerAsParent

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 1, 2017

Member

@rnveach , please review .

Member

romani commented Aug 1, 2017

@rnveach , please review .

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 1, 2017

Member

@romani I see what you are saying but this never seemed like an issue for me since it's name is based on it's inheritance, but users may not know this.

I am not sure what we could rename this to as all our methods are named after the class/interface they extend/implement.
Should we name it isCheckstyleTreeWalkerCheck?
I assume we will not consider renaming modules that end with Check and are actually AbstractFileSets?

Member

rnveach commented Aug 1, 2017

@romani I see what you are saying but this never seemed like an issue for me since it's name is based on it's inheritance, but users may not know this.

I am not sure what we could rename this to as all our methods are named after the class/interface they extend/implement.
Should we name it isCheckstyleTreeWalkerCheck?
I assume we will not consider renaming modules that end with Check and are actually AbstractFileSets?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 1, 2017

Member

Should we name it isCheckstyleTreeWalkerCheck?

to smth like this or exactly like this.

I assume we will not consider renaming modules that end with Check and are actually AbstractFileSets?

Modules are not rename-able.

Member

romani commented Aug 1, 2017

Should we name it isCheckstyleTreeWalkerCheck?

to smth like this or exactly like this.

I assume we will not consider renaming modules that end with Check and are actually AbstractFileSets?

Modules are not rename-able.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 8, 2017

Member

fix is merged

Member

romani commented Dec 8, 2017

fix is merged

@romani romani closed this Dec 8, 2017

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