flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From fhueske <...@git.apache.org>
Subject [GitHub] flink pull request #3547: [FLINK-5653] Add processing time OVER ROWS BETWEEN...
Date Thu, 16 Mar 2017 21:48:28 GMT
Github user fhueske commented on a diff in the pull request:

    https://github.com/apache/flink/pull/3547#discussion_r106513895
  
    --- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/plan/nodes/datastream/DataStreamOverAggregate.scala
---
    @@ -17,34 +17,41 @@
      */
     package org.apache.flink.table.plan.nodes.datastream
     
    -import org.apache.calcite.plan.{RelOptCluster, RelTraitSet}
    +import org.apache.calcite.plan.{ RelOptCluster, RelTraitSet }
    --- End diff --
    
    This PR has several changes that reformat the code but do not change the logic.
    In general, we try to limit PRs to the scope of the issue and avoid reformatting.
    
    1. It makes the PR harder to review because every reformatting change needs to be checked
(was something changed or not). This is especially tiresome when indention is changed.
    2. It makes it more difficult to track changes in the history.
    3. Other users do not necessarily agree with your (or your IDE's) formatting and will
format it back. So it might get back and forth.
    
    There is nothing wrong with adding a space between arguments here and there, but please
keep reformatting changes to a minimum.  


---
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.
---

Mime
View raw message