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

Checkstyle #47

Merged
merged 3 commits into from
Oct 27, 2019
Merged

Checkstyle #47

merged 3 commits into from
Oct 27, 2019

Conversation

krichter722
Copy link
Contributor

@krichter722 krichter722 commented Oct 25, 2019

Merge after #45


This change is Reviewable

@krichter722 krichter722 mentioned this pull request Oct 25, 2019
@keilw keilw merged commit 94fa2e6 into unitsofmeasurement:master Oct 27, 2019
@keilw
Copy link
Member

keilw commented Oct 27, 2019

Somehow the first merge automatically merged all of them, but @krichter722 it seems you broke the build with: [ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.1.0:check (check_style) on project uom-lib: Execution check_style of goal org.apache.maven.plugins:maven-checkstyle-plugin:3.1.0:check failed: Plugin org.apache.maven.plugins:maven-checkstyle-plugin:3.1.0 or one of its dependencies could not be resolved: Failure to find tech.uom.lib:uom-code-analysis:jar:1.0 in https://repo.maven.apache.org/maven2 was cached in the local repository, resolution will not be reattempted until the update interval of central has elapsed or updates are forced ->

What is uom-code-analysis and why would we need such a tooling-only dependency?

@keilw
Copy link
Member

keilw commented Oct 27, 2019

I saw that dependency and it contains nothing but a checkstyle.xml file. @krichter722 Why would we need a module for this, is it the only way the Checkstyle plugin works now? Or does it work fine if the XML file was located e.g. in /src/main/config like for example the configuration of the license plugin?

@keilw keilw mentioned this pull request Oct 27, 2019
@krichter722
Copy link
Contributor Author

@keilw Review is better done before the merge and after the build is green ;) I added comments explaining the dependencies on all PRs. Now, it's a bit messy. I recommend you merge PRs one at a time and wait whether the build is green. Also, if you get yourself in trouble like this revert the commits (if necessary all of them) and start over.

The separate project code-analysis allows to be able to run mvn commands involving checkstyle in the subproject without any duplicated configuration. Alternatives are adding a relative path to the configuration in each submodule or being able to run mvn in the source root only.

@keilw
Copy link
Member

keilw commented Oct 27, 2019

Well that is exactly what I tried but the damn system somehow "miraculously" merged all 4 when I merged just #45. ;-/

If you think it is worth to do this in a reusable way, then code-analysis should not reside here, but under https://github.com/unitsofmeasurement/uom-tools. If you would like to propose a PR for that, there is not a simple way to refactor the sub-module into a different repo like moving issues. It belongs there and having it here as a module and dependency for all including the parent POM would create a big mess. It is already a bit tricky between uom-lib-common and the other libraries which may depend on it, but common rarely changes. Having a plugin dependency here seems less natural than tools which were intended for those design time tools.

@keilw
Copy link
Member

keilw commented Oct 27, 2019

And before applied here, such tools artifact should be published to a Maven repo as well, at least a Snapshot one, or (if tagged 1.0 already) the Release one.

@krichter722 krichter722 deleted the checkstyle branch March 29, 2020 13:52
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

2 participants