You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apex.apache.org by chandnisingh <gi...@git.apache.org> on 2016/01/08 05:34:59 UTC
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
GitHub user chandnisingh opened a pull request:
https://github.com/apache/incubator-apex-core/pull/199
APEXCORE-268 #resolve #comment fixed checkstyle violations in common
@tweise @vrozov please review and merge
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/chandnisingh/incubator-apex-core APEXCORE-268
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-apex-core/pull/199.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #199
----
commit ef6c239380f5de0fff0ae04f3f9f68e2168acb59
Author: MalharJenkins <je...@datatorrent.com>
Date: 2016-01-08T04:31:57Z
APEXCORE-268 #resolve #comment fixed checkstyle violations in common
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170189818
There should be a separate discussion on that since now it is a debate.
I don't think it is a problem to run just another command to see the exact violation.
Don't the users already do that for license, etc?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170419153
@chandnisingh all pull requests conversations are part of dev@apex already, I don't see need to have a duplicate discussion.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170306502
@chandnisingh The result of the vote was to not enforce line length. We cannot have it both ways. Any printing of line length validation warnings is for informational purposes, there is no expectation it will lead to anything being "fixed" and as such it is just noise in the CI logs.
Tuning this can be a separate discussion as you say, this PR was about fixing the violations. There is nothing wrong dealing with a closely related issue as part of it, but when there is no consensus let's decouple it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170123144
Try "mvn verify" or "mvn install". IMO, it is better to display errors for the verify goal and do not require separate invocation of checkstyle:check goal.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/199#discussion_r49236401
--- Diff: common/pom.xml ---
@@ -56,7 +56,7 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<configuration>
- <maxAllowedViolations>114</maxAllowedViolations>
+ <logViolationsToConsole>true</logViolationsToConsole>
--- End diff --
Which mvn goal do you use? I only see errors displayed when I explicitly use checkstyle:check. For the "verify" goal I don't see individual errors printed, only the total number of errors.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170451099
@vrozov is this the workflow we need to follow? Starting a separate discussion isn't required even on topics which do not have a consensus?
Please clarify this as I was under the impression that the workflow consisted of starting a discussion first before creating a JIRA and a pull request.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170188868
@chandnisingh I don't think it is the question of importance. It is the question of convenience for developers. If compiler warnings don't clutter build log, checkstyle warnings also should not clutter.
Will suppression of LineLength warnings be a workable solution? In this case you may add
```
<suppress checks="LineLength" files="" />
```
to checkstyle-suppresions.xml
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by amberarrow <gi...@git.apache.org>.
Github user amberarrow commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170275846
Since command line parameter definitions override those in pom.xml, developers can use aliases to toggle these flags in any desired way, for example:
* alias vfy-none="mvn verify -Dcheckstyle.console=false -Dcheckstyle.consoleOutput=false -DskipTests"
* alias vfy-both="mvn verify -Dcheckstyle.console=true -Dcheckstyle.consoleOutput=true -DskipTests"
* alias vfy-cc="mvn verify -Dcheckstyle.console=true -Dcheckstyle.consoleOutput=false -DskipTests"
* alias vfy-co="mvn verify -Dcheckstyle.console=false -Dcheckstyle.consoleOutput=true -DskipTests"
We can then use these aliases:
vfy-cc > cc.txt
vfy-co > co.txt
vfy-none > none.txt
vfy-both > both.txt
And finally examine the results:
grep -ic warn both.txt none.txt co.txt cc.txt
both.txt:1270
none.txt:0
co.txt:1268
cc.txt:2
grep -ic error both.txt none.txt co.txt cc.txt
both.txt:6564
none.txt:2
co.txt:3283
cc.txt:3283
(I'm not arguing for the decision to go one way or the other, just suggesting that developers can cope with either with minimal inconvenience).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170442523
@chandnisingh Discussion on the pull request already generates multiple emails to dev@apex as well as automatic updates to Apache JIRA. With all these e-mails it becomes difficult to keep up with dev@apex mailing list. As updates to JIRA are posted to dev@apex (with proper subject line, for example `[jira] [Commented] (APEXCORE-268) Fix existing checkstyle violations in common module`), it is sufficient to have pull request and corresponding JIRA open.
As Apache JIRA currently does not support smart commits, dropping `#resolve #comment` will provide more space for the information on the pull request.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170287936
The problem is not that it is impossible to invoke maven with custom parameters and achieve the desired result, IMO question and discussion is about default behavior. IMO, by default errors and warnings for checkstyle should be printed the same way they are printed for compile. This will give the most optimal default behavior that minimizes development effort to fix errors and warnings.
@chandnisingh my suggestion is not to completely disable line length check, but to disable print out of those violations during default mvn invoke while outputting all other errors and warnings. It will be good to make final decision whether or not we allow lines that exceed 120 characters, but this is a separate from this pull request discussion.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170124773
I see. However consoleOutput prints out all the warnings about line lengths.
IMO that will create a lot of clutter.
Here is the output with consoleOutput as true:
```
ARN] /Users/chandni/workspace/Apex/common/src/main/java/com/datatorrent/common/experimental/AppData.java:65: Line is longer than 120 characters (found 123). [LineLength]
[WARN] /Users/chandni/workspace/Apex/common/src/main/java/com/datatorrent/common/experimental/AppData.java:67: Line is longer than 120 characters (found 155). [LineLength]
[WARN] /Users/chandni/workspace/Apex/common/src/main/java/com/datatorrent/common/experimental/AppData.java:68: Line is longer than 120 characters (found 145). [LineLength]
[WARN] /Users/chandni/workspace/Apex/common/src/main/java/com/datatorrent/common/experimental/AppData.java:70: Line is longer than 120 characters (found 161). [LineLength]
[WARN] /Users/chandni/workspace/Apex/common/src/main/java/com/datatorrent/common/experimental/AppData.java:74: Line is longer than 120 characters (found 138). [LineLength]
[WARN] /Users/chandni/workspace/Apex/common/src/main/java/com/datatorrent/common/experimental/AppData.java:83: Line is longer than 120 characters (found 126). [LineLength]
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170134920
with `consoleOutput` I can't find a way to print only errors and not warnings.
with `logViolationsToConsole` only errors are printed when `mvn checkstyle:check` is run.
From what I understand checkstyle:check binds by default to verify phase.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170197381
I prefer Vlad's suggestion to suppress line length warnings over suppressing errors in the output that will directly point the developer to the cause of the build failure, w/o running the extra command.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170135577
@vrozov the reason is same to why we were not printing all the errors earlier because there were too many of them. It is a warning based on the discussion we have at dev@apex.
IMO this should be a separate discussion since the pull request for that change is already merged.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170428652
@vrozov Couple of reasons why I am insisting on starting a discussion on dev
1. There is no consensus here. So the change in the pull request is Not something which everyone has agreed on. From what I see in these cases, is that developers are required to start a discussion so that a consensus is reached.
2. Here is a subject of a pull request email.
`[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...`
These subject lines doesn't help in capturing everyone's attention.
Developers participate in check-style related changes actively so why inhibit it?
I don't see any harm in starting a discussion about this so that we can reach a consensus by involving the community.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170161484
I don't think codestyle warnings should be compared to compiler warnings. IMO the later is much more important.
For now line length is a warning and we have a lot of them. It isn't helpful to print them on console with errors.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/199#discussion_r49212009
--- Diff: common/src/main/java/com/datatorrent/common/util/SerializableObject.java ---
@@ -58,37 +58,29 @@ public Object readResolve() throws ObjectStreamException
Constructor<? extends SerializableObject> constructor = this.getClass().getConstructor(this.getClass());
try {
constructor.setAccessible(true);
- }
- catch (SecurityException ex) {
+ } catch (SecurityException ex) {
logger.warn("Accessing copy constructor {} failed.", constructor, ex);
}
try {
return constructor.newInstance(this);
- }
- catch (InstantiationException ex) {
+ } catch (InstantiationException ex) {
--- End diff --
Multi exception catch and usage of ReflectiveOperationException.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/199#discussion_r49232820
--- Diff: common/pom.xml ---
@@ -56,7 +56,7 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<configuration>
- <maxAllowedViolations>114</maxAllowedViolations>
+ <logViolationsToConsole>true</logViolationsToConsole>
--- End diff --
@vrozov I checked with logViolationsToConsole as true, violations are printed
Please refer to this document.
https://maven.apache.org/plugins/maven-checkstyle-plugin/check-mojo.html
checkstyle.console is the user property for logViolationsToConsole. In our case it is set to false.
consoleOutput will output violations + warnings. The user property associated with that is checkstyle.consoleOutput.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/199#discussion_r49207712
--- Diff: common/src/main/java/com/datatorrent/common/metric/MetricsAggregator.java ---
@@ -129,13 +128,13 @@ public void addAggregators(@NotNull String metric, @NotNull SingleMetricAggregat
* be used for the result of aggregators[i].
*/
public void addAggregators(@NotNull String metric, @NotNull SingleMetricAggregator[] aggregators,
- @NotNull String[] logicalMetricNames)
+ @NotNull String[] logicalMetricNames)
{
Preconditions.checkNotNull(metric, "metric");
Preconditions.checkNotNull(aggregators, "aggregators");
Preconditions.checkNotNull(logicalMetricNames, "logicalMetricNames");
Preconditions.checkArgument(aggregators.length == logicalMetricNames.length, "different length aggregators and" +
- " logical names");
+ " logical names");
--- End diff --
Avoid split on the string literal. Can the previous line be split after the comma?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/199#discussion_r49218896
--- Diff: common/pom.xml ---
@@ -56,7 +56,7 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<configuration>
- <maxAllowedViolations>114</maxAllowedViolations>
+ <logViolationsToConsole>true</logViolationsToConsole>
--- End diff --
Please refer at the parent pom. logViolationsToConsole is controlled by the user property checkstyle.console which by default is false
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-apex-core/pull/199
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170304790
@vrozov if we follow the suggestion line-length warnings with user property `consoleOutput` will not be printed which are currently being printed. Even though this was added recently but it is a change which is not related to APEXCORE-268. If that wasn't discussed with community it was a mistake which we should not carry on.
This should be discussed with a wider audience and the community should make a decision because developers which wanted line length check, may want it.
This pull request is specifically for removing violations for common module.
As I mentioned before, with this pull request I am not adding any additional steps. Since it is a debate now, what should be printed and how should be discussed with Apex community.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170381581
@vrozov please start a discussion on dev@apex so that a wider audience is involved. It will be worthwhile to see what everyone else thinks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/199#discussion_r49218127
--- Diff: common/pom.xml ---
@@ -56,7 +56,7 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<configuration>
- <maxAllowedViolations>114</maxAllowedViolations>
+ <logViolationsToConsole>true</logViolationsToConsole>
--- End diff --
<logViolationsToConsole> option is true by default. Please set <consoleOutput> to true and squash commits.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170138204
squashed the commits.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170147744
Agree that whether or not "recommendation" should translate to warnings or not is a different topic.
As compile prints both warnings and errors, I think it should be OK to print both errors and warnings for checkstyle as well and we should not require extra step.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170144697
I suggest that "mvn verify" prints both errors and warnings, this way warnings will be fixed overtime, otherwise warnings will never be fixed and more warnings will be introduced.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170200390
I don't agree with it. A lot of folks in the community wanted fixed line length which is why I added it as a warning. That pull request was merged without any objections. So why suppress the warnings on an unrelated change.
With this change it's not that I am increasing the steps. They are same as what they were.
This is a separate topic and the whole community should participate in making that decision
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170126536
Can it be configured to only print errors?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170307593
@tweise These objections are misplaced and not related to this issue. They should have been made on the pull request which reduced the line length severity and I mentioned that it was a mistake that it wasn't discussed then there and so we should not continue it and involve the community.
Currently CI logs do not print these warnings so I don't understand which noise you are referring to.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by tweise <gi...@git.apache.org>.
Github user tweise commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170146555
I think it is helpful to see the errors in the log because they will fail the build and it is an extra step (albeit not a huge deal) for the submitter to run checkstyle:check -Dblablabla locally (actually had added that to the contributor guidelines:).
Whether "recommendations" should be warnings is perhaps a separate topic?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170589876
@chandnisingh It depends on impact and complexity of the changes. IMO, it is an overkill to start a discussion on small bug fixes, maven plugin upgrades or, like in this case, maven plugin configuration changes that can be easily be reverted. Roadmap features and changes to the checkstyle rules need to be discussed on dev@apex first as they have a much larger impact.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170374609
I will open a separate pull request to discuss default maven checkstyle plugin configuration.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170134683
What is the reason to suppress warnings? Do we want them to be fixed over time? If not, should warning rule be disabled completely?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/199#discussion_r49213035
--- Diff: common/pom.xml ---
@@ -56,7 +56,7 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<configuration>
- <maxAllowedViolations>114</maxAllowedViolations>
+ <maxAllowedViolations>0</maxAllowedViolations>
--- End diff --
Drop default 0 and enable console output.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/199#discussion_r49220118
--- Diff: common/pom.xml ---
@@ -56,7 +56,7 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<configuration>
- <maxAllowedViolations>114</maxAllowedViolations>
+ <logViolationsToConsole>true</logViolationsToConsole>
--- End diff --
I do see different behavior depending on whether logViolationsToConsole or consoleOutput is set to true in the pom file. Without consoleOutput set to true only total number of violations is printed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170121349
I am using mvn checkstyle:check -DskipTests
I have introduced 1 violation
Please see the output - first line
```
[ERROR] src/main/java/com/datatorrent/common/metric/MetricsAggregator.java:[141,119] (blocks) LeftCurly: '{' at column 119 should be on a new line.
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO]
[INFO] Apache Apex (incubating) ........................... SUCCESS [ 1.024 s]
[INFO] Apache Apex (incubating) API ....................... SUCCESS [ 1.203 s]
[INFO] Apache Apex (incubating) Common Library ............ FAILURE [ 0.458 s]
[INFO] Apache Apex (incubating) Buffer Server ............. SKIPPED
[INFO] Apache Apex (incubating) Stream Processing Engine .. SKIPPED
[INFO] Apache Apex (incubating) Application Maven Archetype SKIPPED
[INFO] Apache Apex (incubating) App Configuration Maven Archetype SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 3.210 s
[INFO] Finished at: 2016-01-08T12:53:21-08:00
[INFO] Final Memory: 23M/307M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:2.17:check (default-cli) on project apex-common: You have 1 Checkstyle violation. -> [Help 1]
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEXCORE-268 #resolve #comment f...
Posted by chandnisingh <gi...@git.apache.org>.
Github user chandnisingh commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/199#issuecomment-170065055
@vrozov Made the changes. please review
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---