ImportControl: allow to load files from resources #3450

Closed
romani opened this Issue Sep 19, 2016 · 8 comments

Comments

Projects
None yet
2 participants
@romani
Member

romani commented Sep 19, 2016

base on #3358

ImportControl does not appear to be able to find configurations that are resources, only files. So this does not seem to be usable at all if checkstyle.xml is in your project as a resource rather than a file.


Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Nov 18, 2016

Contributor

I will implement this.

Contributor

jochenvdv commented Nov 18, 2016

I will implement this.

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Nov 19, 2016

Contributor

Should the resource loading be like the way it is done in for example SuppressionFilter?

Contributor

jochenvdv commented Nov 19, 2016

Should the resource loading be like the way it is done in for example SuppressionFilter?

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Nov 24, 2016

Contributor

SuppresionFilter has a single property file which is interpreted as a filename or URL and resource as a fallback method,
but ImportControlCheck has already separate properties for file and URL.

Shall I make a separate property resource which accepts a path like my/package/resource.ext?

<module name="ImportControl">
    <property name="resource" value="/my/package/import-control.xml"/>
</module>

Or shall I use the file property and fall back to trying to load as a resource if the file could not be found?

<module name="ImportControl">
    <!-- File my/package/import-control.xml not found, so interpreted as resource in classpath -->
    <property name="file" value="/my/package/import-control.xml"/>
</module>

BTW, the file you link loads Checkstyle's main configuration, but the checks seem to use their own implementation which also extends AbstractLoader (see ImportControlLoader).

When CommonUtils.getUriByFilename() is used, it tries to load as a file, URL or resource, so implementing it this way would be easiest but inconsistent with having separate properties file, url and resource.

We could unite those into a single property but that would be a backwards incompatible change.

Tell me what you think. For now I have started implementing this using a new resource property. It is trivial to implement so I will make a PR soon.

Contributor

jochenvdv commented Nov 24, 2016

SuppresionFilter has a single property file which is interpreted as a filename or URL and resource as a fallback method,
but ImportControlCheck has already separate properties for file and URL.

Shall I make a separate property resource which accepts a path like my/package/resource.ext?

<module name="ImportControl">
    <property name="resource" value="/my/package/import-control.xml"/>
</module>

Or shall I use the file property and fall back to trying to load as a resource if the file could not be found?

<module name="ImportControl">
    <!-- File my/package/import-control.xml not found, so interpreted as resource in classpath -->
    <property name="file" value="/my/package/import-control.xml"/>
</module>

BTW, the file you link loads Checkstyle's main configuration, but the checks seem to use their own implementation which also extends AbstractLoader (see ImportControlLoader).

When CommonUtils.getUriByFilename() is used, it tries to load as a file, URL or resource, so implementing it this way would be easiest but inconsistent with having separate properties file, url and resource.

We could unite those into a single property but that would be a backwards incompatible change.

Tell me what you think. For now I have started implementing this using a new resource property. It is trivial to implement so I will make a PR soon.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 24, 2016

Member

but ImportControlCheck has already separate properties for file and URL.

it is legacy. It exists only because we can not easily remove property without breaking backward compatibility. That is why we wait for major release to do this. Please create separate issue on this , i will put "checkstyle8" label on it.
Please use "file", it ok to use CommonUtils. If there is some own implementation of loading - create separate issue.

Member

romani commented Nov 24, 2016

but ImportControlCheck has already separate properties for file and URL.

it is legacy. It exists only because we can not easily remove property without breaking backward compatibility. That is why we wait for major release to do this. Please create separate issue on this , i will put "checkstyle8" label on it.
Please use "file", it ok to use CommonUtils. If there is some own implementation of loading - create separate issue.

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Nov 24, 2016

Contributor

OK. I will change the implementation of the file property to use CommonUtils. This will make it able to load files, URLs and resources. I will document it as well.

I will create a separate issue for removing the url property.
Should I already document it as deprecated in my PR or do this in a separate PR?

Contributor

jochenvdv commented Nov 24, 2016

OK. I will change the implementation of the file property to use CommonUtils. This will make it able to load files, URLs and resources. I will document it as well.

I will create a separate issue for removing the url property.
Should I already document it as deprecated in my PR or do this in a separate PR?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 24, 2016

Member

Should I already document it as deprecated in my PR or do this in a separate PR?

to mark as deprecated please do by separate issue to explain the reason and prove that certain property is odd. Our releasenotes have links to issues, there all details should be.

Member

romani commented Nov 24, 2016

Should I already document it as deprecated in my PR or do this in a separate PR?

to mark as deprecated please do by separate issue to explain the reason and prove that certain property is odd. Our releasenotes have links to issues, there all details should be.

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Nov 25, 2016

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Nov 27, 2016

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Nov 28, 2016

romani added a commit that referenced this issue Nov 28, 2016

@romani romani added the new feature label Nov 28, 2016

@romani romani added this to the 7.4 milestone Nov 28, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 28, 2016

Member

fix is merged.

Member

romani commented Nov 28, 2016

fix is merged.

@romani romani closed this Nov 28, 2016

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