You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by dawidwys <gi...@git.apache.org> on 2017/05/29 13:26:08 UTC
[GitHub] flink pull request #4009: [FLINK-6732] Activate strict-checkstyle for flink-...
GitHub user dawidwys opened a pull request:
https://github.com/apache/flink/pull/4009
[FLINK-6732] Activate strict-checkstyle for flink-java
Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the [How To Contribute guide](http://flink.apache.org/how-to-contribute.html).
In addition to going through the list, please provide a meaningful description of your changes.
- [ ] General
- The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
- The pull request addresses only one issue
- Each commit in the PR has a meaningful commit message (including the JIRA id)
- [ ] Documentation
- Documentation has been added for new functionality
- Old documentation affected by the pull request has been updated
- JavaDoc for public methods has been added
- [ ] Tests & Build
- Functionality added by the pull request is covered by tests
- `mvn clean verify` has been executed successfully locally or a Travis build has passed
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dawidwys/flink flink-java-checkstyle
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4009.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 #4009
----
commit 55e59f49d6487e0c90fa5896cb3090129ad7bd91
Author: Dawid Wysakowicz <da...@getindata.com>
Date: 2017-05-29T13:15:54Z
[FLINK-6732] Removed trailing whitespaces
commit b4ec5f0642281e4916efc781729a124f743253cf
Author: Dawid Wysakowicz <da...@getindata.com>
Date: 2017-05-26T13:04:05Z
[FLINK-6734] Exclude org.apache.flink.api.java.tuple from checkstyle AvoidStarImport
commit 3a849de2d0291a4ba90c6466b1c898bde94e12d0
Author: Dawid Wysakowicz <da...@getindata.com>
Date: 2017-05-26T13:09:42Z
[FLINK-6732] Reorganized imports
commit 2eaae31c7fccfc052303c032d8a98a31319966dc
Author: Dawid Wysakowicz <da...@getindata.com>
Date: 2017-05-28T08:50:37Z
[FLINK-6732] Fixed empty lines issues
commit 0e136e81f1c9036993ab5159fee3fc0addf7291b
Author: Dawid Wysakowicz <da...@getindata.com>
Date: 2017-05-28T10:08:28Z
[FLINK-6732] Fixed existing javadocs
commit 202aa35aea2501ec75d21a8762c04880dae63f5f
Author: Dawid Wysakowicz <da...@getindata.com>
Date: 2017-05-29T13:23:22Z
[FLINK-6732] Added missing javadocs
----
---
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] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on the issue:
https://github.com/apache/flink/pull/4009
I've created set of changes that remove ~3k checkstyle violations in flink-java.
I had doubts about changing some `public static final classes` into `private static final classes` e.g in `MinAggregationFunction.java`. Is it safe to do so? Can I check it does not interfere with any part of the system. Or should I just revert it and add appropriate javadoc?
@zentol What is your opinion?
---
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] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4009
I was wondering whether we want to extend this PR to cover all checkstyle rules at once. We are touching so much already, might as well go all-in.
We should also try to group the changes by package to make the reviewing easier imo.
---
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] flink pull request #4009: [FLINK-6732] Activate strict-checkstyle for flink-...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4009#discussion_r118941471
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/utils/DataSetUtils.java ---
@@ -141,7 +141,7 @@ public void mapPartition(Iterable<T> values, Collector<Tuple2<Long, T>> out) thr
* <li> c is shifted by n bits where n = log2(number of parallel tasks)
* <li> to create a unique ID among all tasks, the task id is added to the counter
* <li> for each record, the resulting counter is collected
- * </ul>
+ * </ul>.
--- End diff --
This will probably render weird in the final javadocs. Let's reword the first sentence slightly; i.e. replace "the following way:" -> "as described below."
---
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] flink pull request #4009: [FLINK-6732] Activate strict-checkstyle for flink-...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4009#discussion_r118946288
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/aggregation/UnsupportedAggregationTypeException.java ---
@@ -20,6 +20,9 @@
import org.apache.flink.annotation.PublicEvolving;
+/**
+ * Exception indicate an unsupported type was used for aggregations.
--- End diff --
-> "indicating an", "used for an aggregation"
---
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] flink pull request #4009: [FLINK-6732] Activate strict-checkstyle for flink-...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4009#discussion_r118946203
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/aggregation/SumAggregationFunction.java ---
@@ -26,6 +26,10 @@
import org.apache.flink.types.LongValue;
import org.apache.flink.types.ShortValue;
+/**
+ * Definitions of sum function for different numerical types.
--- End diff --
-> "of sum functions"
---
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] flink pull request #4009: [FLINK-6732] Activate strict-checkstyle for flink-...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4009#discussion_r118940089
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/operators/SingleInputUdfOperator.java ---
@@ -114,41 +114,41 @@ public O withBroadcastSet(DataSet<?> data, String name) {
}
/**
- * <p>
- * Adds semantic information about forwarded fields of the user-defined function.
+ *
+ * <p>Adds semantic information about forwarded fields of the user-defined function.
* The forwarded fields information declares fields which are never modified by the function and
* which are forwarded at the same position to the output or unchanged copied to another position in the output.
* </p>
--- End diff --
`</p>` tags should be removed.
---
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] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on the issue:
https://github.com/apache/flink/pull/4009
Okay, I now see there are many outstanding violations which look to be the type fixable by regex. If we split out PRs by package we should by able to copy over the comments updates from this PR as necessary.
---
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] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4009
eh...the commits just jumped around so i assume you just force pushed? From this point on please don't.
As for kryo, well you got me there. Frankly i don't know where we use kryo or something else; so far i relied on failing tests to tell me that.
---
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] flink pull request #4009: [FLINK-6732] Activate strict-checkstyle for flink-...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4009#discussion_r118948338
--- Diff: flink-java/src/test/java/org/apache/flink/api/common/operators/base/ReduceOperatorTest.java ---
@@ -47,6 +47,9 @@
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
+/**
+ * Tests for {@link ReduceFunction}.
--- End diff --
should link to `ReduceOperator` instead
---
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] flink pull request #4009: [FLINK-6732] Activate strict-checkstyle for flink-...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4009#discussion_r118946760
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/functions/FormattingMapper.java ---
@@ -23,6 +23,10 @@
import org.apache.flink.api.java.io.TextOutputFormat;
import org.apache.flink.api.java.io.TextOutputFormat.TextFormatter;
+/**
+ * Mapper that formats value into {@link String} representation.
--- End diff --
-> "Mapper that converts values to strings using a {@link TextFormatter}."
I don't think we need to link to the String class ;)
---
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] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on the issue:
https://github.com/apache/flink/pull/4009
Hi @greghogan @zentol I will rebase this PR, fix remaining checkstyle violations and split into PRs corresponding to packages. Is that ok?
---
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] flink pull request #4009: [FLINK-6732] Activate strict-checkstyle for flink-...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys closed the pull request at:
https://github.com/apache/flink/pull/4009
---
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] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on the issue:
https://github.com/apache/flink/pull/4009
Right, but won't there be any serialization issues? I found a comment like which made me ask that question:
`// Nested classes are only "public static" for Kryo serialization, otherwise they'd be private`
---
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] flink pull request #4009: [FLINK-6732] Activate strict-checkstyle for flink-...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4009#discussion_r118940280
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/operators/TwoInputUdfOperator.java ---
@@ -192,41 +192,41 @@ public O withForwardedFieldsFirst(String... forwardedFieldsFirst) {
}
/**
- * <p>
- * Adds semantic information about forwarded fields of the second input of the user-defined function.
+ *
+ * <p>Adds semantic information about forwarded fields of the second input of the user-defined function.
--- End diff --
same as above
---
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] flink pull request #4009: [FLINK-6732] Activate strict-checkstyle for flink-...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4009#discussion_r118948292
--- Diff: flink-java/src/test/java/org/apache/flink/api/common/operators/base/InnerJoinOperatorBaseTest.java ---
@@ -45,6 +45,9 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.fail;
+/**
+ * Tests for {@link FlatJoinFunction}.
--- End diff --
should link to `InnerJoinOperatorBase` instead.
---
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] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4009
If a function is tagged as `@Internal` or in a class tagged as such then we should be able to restrict the visibility. Would be good if you could reply in the JIRA for the tuple checkstyle exclusion.
---
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] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on the issue:
https://github.com/apache/flink/pull/4009
Yes sorry for that, Just wanted to remove the `AvoidStarImport` commit from in the middle. Any further changes will do with new 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] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on the issue:
https://github.com/apache/flink/pull/4009
@zentol I believe this is the only outstanding checkstyle PR. Is this still in your review queue?
---
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] flink issue #4009: [FLINK-6732] Activate strict-checkstyle for flink-java
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4009
I've noticed some `<br>` tags here and there, could replace those as well.
---
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] flink pull request #4009: [FLINK-6732] Activate strict-checkstyle for flink-...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4009#discussion_r118947281
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/functions/GroupReduceIterator.java ---
@@ -24,6 +24,11 @@
import java.util.Iterator;
+/**
+ * Base class that simplifies reducing all values provided as {@link Iterable}.
--- End diff --
-> "A {@link RichGroupReduceFunction}" that returns an {@link Iterable} instead."
---
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] flink pull request #4009: [FLINK-6732] Activate strict-checkstyle for flink-...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4009#discussion_r118943716
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/DataSet.java ---
@@ -1666,29 +1666,29 @@ public void printToErr() throws Exception {
/**
* Writes a DataSet to the standard error stream (stderr).
- *
+ *
* <p>For each element of the DataSet the result of {@link Object#toString()} is written.</p>
*
* @param sinkIdentifier The string to prefix the output with.
* @return The DataSink that writes the DataSet.
- *
- * @deprecated Use {@link #printOnTaskManager(String)} instead, othe
+ *
+ * @deprecated Use {@link #printOnTaskManager(String)} instead, othe
--- End diff --
"othe" -> "or the", removing second "instead"
---
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] flink pull request #4009: [FLINK-6732] Activate strict-checkstyle for flink-...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4009#discussion_r118939832
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/io/LocalCollectionOutputFormat.java ---
@@ -33,7 +33,7 @@
import java.util.Map;
/**
- * An output format that writes record into collection
+ * An output format that writes record into collection.
--- End diff --
-> "writes records into a collection", or "adds records to a collection"
---
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] flink pull request #4009: [FLINK-6732] Activate strict-checkstyle for flink-...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4009#discussion_r118946444
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/functions/FirstReducer.java ---
@@ -23,6 +23,10 @@
import org.apache.flink.api.common.functions.GroupReduceFunction;
import org.apache.flink.util.Collector;
+/**
+ * Reducer that collects provided number of elements in a group.
--- End diff --
-> "Reducer that only emits the first N elements in a group."
---
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] flink pull request #4009: [FLINK-6732] Activate strict-checkstyle for flink-...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4009#discussion_r118940189
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/operators/SingleInputUdfOperator.java ---
@@ -114,41 +114,41 @@ public O withBroadcastSet(DataSet<?> data, String name) {
}
/**
- * <p>
- * Adds semantic information about forwarded fields of the user-defined function.
+ *
+ * <p>Adds semantic information about forwarded fields of the user-defined function.
--- End diff --
`<p>` should be removed as this is the first sentence. Empty line above should be removed as well.
---
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] flink pull request #4009: [FLINK-6732] Activate strict-checkstyle for flink-...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4009#discussion_r118946093
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/aggregation/Aggregations.java ---
@@ -20,6 +20,9 @@
import org.apache.flink.annotation.Public;
+/**
+ * Shortcuts for Aggregations factories.
--- End diff --
-> "Aggregation factories"
---
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] flink pull request #4009: [FLINK-6732] Activate strict-checkstyle for flink-...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4009#discussion_r118940257
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/operators/TwoInputUdfOperator.java ---
@@ -117,41 +117,41 @@ public O withBroadcastSet(DataSet<?> data, String name) {
}
/**
- * <p>
- * Adds semantic information about forwarded fields of the first input of the user-defined function.
+ *
+ * <p>Adds semantic information about forwarded fields of the first input of the user-defined function.
--- End diff --
same as above
---
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.
---