Skip to content
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

Clean up to remove <subpackage-s from import-control.xml #484

Closed

Conversation

vorburger
Copy link
Contributor

These appear to be left over from another project, or by gone days...
the current src/ code of this project (sevntu-checks) does not have any
such packages.

It's causing no real harm, but it's initially confusing to read the
import-control.xml when something goes wrong in it to see all these
useless declarations.

Signed-off-by: Michael Vorburger mike@vorburger.ch

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.218% when pulling ceec467 on vorburger:importControlCleanUp into d77b072 on sevntu-checkstyle:master.

@romani
Copy link
Member

romani commented Nov 17, 2016

Yes , this is a copy from main project with minor correction for package names.

Original idea was too keep rules the same as main project to ease future transition of Check to main project.

Complete removal is not accepted. It is valuable validation in general. Even @rnveach run into it with bad update :).

I am ok to remove subpackages except for "checks" and"filter". Rules that are left could be updated to be uptodate and match what Checks need.

@romani
Copy link
Member

romani commented Nov 22, 2016

@vorburger , ping.
Code change is required.

@rnveach
Copy link
Contributor

rnveach commented Nov 22, 2016

@vorburger Please be aware that i just changed the import file in a previous PR.

These appear to be left over from another project, or by gone days...
the current src/ code of this project (sevntu-checks) does not have any
such packages.

It's causing no real harm, but it's initially confusing to read the
import-control.xml when something goes wrong in it to see all these
useless declarations.

Signed-off-by: Michael Vorburger <mike@vorburger.ch>
@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.289% when pulling 0b98037 on vorburger:importControlCleanUp into 29b5ac0 on sevntu-checkstyle:master.

@vorburger
Copy link
Contributor Author

vorburger commented Nov 22, 2016

@rnveach OK, I've rebased it (there were no conflicts)

@romani pong .. I'm afraid even after re-reading previous comments above, I'm struggling to understand what code change you would like to see here.. I've tentatively put the <subpackage name="checks"> back, without the subpackages indentation, header & javadoc - because there are no such packages! Likewise for "filter" - I don't see what you mean (again, there is no sub-packaged named "filter" in sevntu-checks, that I can see).

If this is still NOK, given that you seem to have a very clear idea of what you would like to see here, perhaps it would be easiest if you just went ahead and amended this Pull Request. I've just checked that (new) GitHub feature to [X] Allow edits from maintainers on this.

If that doesn't work for you, I'm tempted to give up and close this Pull Request un-merged; I'd like to focus my energy contributing to Checkstyle on #457 related work.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.289% when pulling a1590da on vorburger:importControlCleanUp into 29b5ac0 on sevntu-checkstyle:master.

@rnveach
Copy link
Contributor

rnveach commented Nov 22, 2016

Just my opinion, but I would say restore everything in checks and their sub-packages, and filters.
Even if the packages or sub-package don't exist, it doesn't mean someone wont be adding new checks to those areas in the future.
This is test area for new checks before they move to main checkstyle project. So this project will hold only customized stuff for checkstyle. I don't see anyone making a custom ant, api, doclets, or gui.

@romani
Copy link
Member

romani commented Nov 22, 2016

done in 7b6e54f .

@romani romani closed this Nov 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants