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

Split and Organize Checkstyle inputs by Test for checks in 'ant' package #4686

Closed
subkrish opened this Issue Jul 9, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@subkrish
Contributor

subkrish commented Jul 9, 2017

subtask of #3891

Move / Make a copy of all test input files to a separate folder for all checks in 'ant' package.
Not any other Check should use new inputs.

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jul 9, 2017

Contributor

I'm on it.

There is only one file in this package -> CheckstyleAntTaskTest. Its inputs will be moved to checkstyleanttask directory

Contributor

subkrish commented Jul 9, 2017

I'm on it.

There is only one file in this package -> CheckstyleAntTaskTest. Its inputs will be moved to checkstyleanttask directory

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 9, 2017

Issue #4686: Modified CheckstyleAntTaskTest.java and moved its input …
…files to the 'checkstyleanttask' subdirectory

@rnveach rnveach added the approved label Jul 9, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 9, 2017

Issue #4686: Modified CheckstyleAntTaskTest.java and moved its input …
…files to the 'checkstyleanttask' subdirectory

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 9, 2017

Issue #4686: Modified CheckstyleAntTaskTest.java and moved its input …
…files to the 'checkstyleanttask' subdirectory

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 10, 2017

Issue #4686: Modified CheckstyleAntTaskTest.java and moved its input …
…files to the 'checkstyleanttask' subdirectory

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 10, 2017

Issue #4686: Modified CheckstyleAntTaskTest.java and moved its input …
…files to the 'checkstyleanttask' subdirectory
@romani

This comment has been minimized.

Show comment
Hide comment

@romani romani removed the approved label Jul 10, 2017

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jul 10, 2017

Contributor

@romani

I feel the reason here is that all the inputs need to belong to one folder. For example, config_custom_root_module.xml lies outside the folder ant instead of being in it although it is being used by CheckstyleAntTaskTest.

One thing we could do is move the inputs into the ant folder as you have suggested. This would help us to use the getPackageLocation() under #4592 (comment) .

Since the inputs are scattered in different folders when I add the path in the method I am forced to leave it com/puppycrawl/tools/checkstyle instead of com/puppycrawl/tools/checkstyle/ant.

Contributor

subkrish commented Jul 10, 2017

@romani

I feel the reason here is that all the inputs need to belong to one folder. For example, config_custom_root_module.xml lies outside the folder ant instead of being in it although it is being used by CheckstyleAntTaskTest.

One thing we could do is move the inputs into the ant folder as you have suggested. This would help us to use the getPackageLocation() under #4592 (comment) .

Since the inputs are scattered in different folders when I add the path in the method I am forced to leave it com/puppycrawl/tools/checkstyle instead of com/puppycrawl/tools/checkstyle/ant.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 10, 2017

Member

whole ant folder is belong to one Test .

@romani I am fine with it one folder, but it does goes against how we are organizing literally everything else in the main issue.
Also, if we ever need a new class in this package, public or otherwise, we would have to switch back to sub-folders.

Member

rnveach commented Jul 10, 2017

whole ant folder is belong to one Test .

@romani I am fine with it one folder, but it does goes against how we are organizing literally everything else in the main issue.
Also, if we ever need a new class in this package, public or otherwise, we would have to switch back to sub-folders.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 10, 2017

Member

if we ever need a new class in this package

It is not changed for many years, I doubt it will change in future. Let's think about it when future show us their requirements. Let's stay with one folder ( no subfolders) for now.

One thing we could do is move the inputs into the ant folder as you have suggested.

Please do.

For example, config_custom_root_module.xml lies outside the folder ant

Please force all inputs/configs to be in one folder - ant. I am ok to keep copies of some files, but all of them should belong to ant.

Member

romani commented Jul 10, 2017

if we ever need a new class in this package

It is not changed for many years, I doubt it will change in future. Let's think about it when future show us their requirements. Let's stay with one folder ( no subfolders) for now.

One thing we could do is move the inputs into the ant folder as you have suggested.

Please do.

For example, config_custom_root_module.xml lies outside the folder ant

Please force all inputs/configs to be in one folder - ant. I am ok to keep copies of some files, but all of them should belong to ant.

@romani romani added the approved label Jul 10, 2017

romani added a commit that referenced this issue Jul 10, 2017

Issue #4686: Modified CheckstyleAntTaskTest.java and moved its input …
…files to the 'checkstyleanttask' subdirectory
@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 10, 2017

Member

@subkrish Sorry, one more change by me and @romani .
https://github.com/checkstyle/checkstyle/tree/master/src/test/resources/com/puppycrawl/tools/checkstyle/ant/checkstyleanttask
Please move all inputs in flawless directory to checkstyleanttask.

Member

rnveach commented Jul 10, 2017

@subkrish Sorry, one more change by me and @romani .
https://github.com/checkstyle/checkstyle/tree/master/src/test/resources/com/puppycrawl/tools/checkstyle/ant/checkstyleanttask
Please move all inputs in flawless directory to checkstyleanttask.

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 11, 2017

Issue #4686: Moved input of 'flawless' subdirectory to 'checkstyleant…
…task' subdirectory of CheckstyleAntTaskTest

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 11, 2017

Issue #4686: Moved input of 'flawless' subdirectory to 'checkstyleant…
…task' subdirectory of CheckstyleAntTaskTest

subkrish added a commit to subkrish/checkstyle that referenced this issue Jul 11, 2017

Issue #4686: Moved input of 'flawless' subdirectory to 'checkstyleant…
…task' subdirectory of CheckstyleAntTaskTest

romani added a commit that referenced this issue Jul 12, 2017

Issue #4686: Moved input of 'flawless' subdirectory to 'checkstyleant…
…task' subdirectory of CheckstyleAntTaskTest
@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jul 12, 2017

Contributor

@rnveach @romani I think this issue is complete and can be closed.

Contributor

subkrish commented Jul 12, 2017

@rnveach @romani I think this issue is complete and can be closed.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 12, 2017

Member

Fixes were merged

Member

rnveach commented Jul 12, 2017

Fixes were merged

@rnveach rnveach closed this Jul 12, 2017

@rnveach rnveach added this to the 8.1 milestone Jul 12, 2017

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