You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2020/10/28 11:22:21 UTC
[GitHub] [flink] pnowojski opened a new pull request #13820: [FLINK-19671][codestyle] Revert .editorconfig change violating our coding style
pnowojski opened a new pull request #13820:
URL: https://github.com/apache/flink/pull/13820
https://flink.apache.org/contributing/code-style-and-quality-formatting.html#breaking-the-lines-of-too-long-statements
> Rules about breaking the long lines:
>
> Break the argument list or chain of calls if the line exceeds limit or earlier if you believe that the breaking would improve the code readability
> If you break the line then each argument/call should have a separate line, including the first one
> Each new line should have one extra indentation (or two for a function declaration) relative to the line of the parent function name or the called entity
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (yes / **no**)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes / **no**)
- The serializers: (yes / **no** / don't know)
- The runtime per-record code paths (performance sensitive): (yes / **no** / don't know)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn/Mesos, ZooKeeper: (yes / **no** / don't know)
- The S3 file system connector: (yes / **no** / don't know)
## Documentation
- Does this pull request introduce a new feature? (yes / **no**)
- If yes, how is the feature documented? (**not applicable** / docs / JavaDocs / not documented)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] pnowojski merged pull request #13820: [FLINK-19671][codestyle] Revert .editorconfig change violating our coding style
Posted by GitBox <gi...@apache.org>.
pnowojski merged pull request #13820:
URL: https://github.com/apache/flink/pull/13820
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] pnowojski edited a comment on pull request #13820: [FLINK-19671][codestyle] Revert .editorconfig change violating our coding style
Posted by GitBox <gi...@apache.org>.
pnowojski edited a comment on pull request #13820:
URL: https://github.com/apache/flink/pull/13820#issuecomment-718059114
There was a lot of discussions around this topic:
- in the ticket itself
- in two mailing lists discussions referenced in the ticket
- some offline discussion between me @zentol and @aljoscha
Generally speaking, I'm personally fine with changing this to double indentations, but let's change the code style first and me and @zentol are against a gradual change (we would prefer to reformat the whole code base at once)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #13820: [FLINK-19671][codestyle] Revert .editorconfig change violating our coding style
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #13820:
URL: https://github.com/apache/flink/pull/13820#issuecomment-717892896
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c0e721f67b0b4b21b5080900954df10a05890d87",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=8506",
"triggerID" : "c0e721f67b0b4b21b5080900954df10a05890d87",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* c0e721f67b0b4b21b5080900954df10a05890d87 Azure: [FAILURE](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=8506)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #13820: [FLINK-19671][codestyle] Revert .editorconfig change violating our coding style
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #13820:
URL: https://github.com/apache/flink/pull/13820#issuecomment-717870794
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.
## Automated Checks
Last check on commit c0e721f67b0b4b21b5080900954df10a05890d87 (Wed Oct 28 11:25:36 UTC 2020)
**Warnings:**
* No documentation files were touched! Remember to keep the Flink docs up to date!
<sub>Mention the bot in a comment to re-run the automated checks.</sub>
## Review Progress
* ❓ 1. The [description] looks good.
* ❓ 2. There is [consensus] that the contribution should go into to Flink.
* ❓ 3. Needs [attention] from.
* ❓ 4. The change fits into the overall [architecture].
* ❓ 5. Overall code [quality] is good.
Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process.<details>
The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required <summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`)
- `@flinkbot approve all` to approve all aspects
- `@flinkbot approve-until architecture` to approve everything until `architecture`
- `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention
- `@flinkbot disapprove architecture` to remove an approval you gave earlier
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] StephanEwen commented on pull request #13820: [FLINK-19671][codestyle] Revert .editorconfig change violating our coding style
Posted by GitBox <gi...@apache.org>.
StephanEwen commented on pull request #13820:
URL: https://github.com/apache/flink/pull/13820#issuecomment-718096683
Fine by me. I think reformatting the code base makes only sense in combination with adding a checkstyle rule, otherwise it will not have a lasting effect and we pay the merge conflict price for a not-so-great outcome.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] pnowojski commented on pull request #13820: [FLINK-19671][codestyle] Revert .editorconfig change violating our coding style
Posted by GitBox <gi...@apache.org>.
pnowojski commented on pull request #13820:
URL: https://github.com/apache/flink/pull/13820#issuecomment-718059114
There was a lot of discussions around this topic:
- in the ticket itself
- in two mailing lists discussions referenced in the ticket
- some offline discussion between me @zentol and @aljoscha
Generally speaking, I'm fine with changing this to double indentations, but let's change the code style first and me and @zentol are against a gradual change (we would prefer to reformat the whole code base at once)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #13820: [FLINK-19671][codestyle] Revert .editorconfig change violating our coding style
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #13820:
URL: https://github.com/apache/flink/pull/13820#issuecomment-717892896
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c0e721f67b0b4b21b5080900954df10a05890d87",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=8506",
"triggerID" : "c0e721f67b0b4b21b5080900954df10a05890d87",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* c0e721f67b0b4b21b5080900954df10a05890d87 Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=8506)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] aljoscha commented on pull request #13820: [FLINK-19671][codestyle] Revert .editorconfig change violating our coding style
Posted by GitBox <gi...@apache.org>.
aljoscha commented on pull request #13820:
URL: https://github.com/apache/flink/pull/13820#issuecomment-718697677
I still maintain that the `.editorconfig` file was good as is: Out of the box, you got a setup with import ordering and code style that worked and passed our checkstyle and that quite some people already use anyways. That was better than what we had before.
With this change all indentation is now only 1 level which is not very helpful.
(The code base is not consistent when it comes to indentation and with this config it would mean that new people would start producing a consistent style.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #13820: [FLINK-19671][codestyle] Revert .editorconfig change violating our coding style
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #13820:
URL: https://github.com/apache/flink/pull/13820#issuecomment-717892896
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c0e721f67b0b4b21b5080900954df10a05890d87",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "c0e721f67b0b4b21b5080900954df10a05890d87",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* c0e721f67b0b4b21b5080900954df10a05890d87 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] StephanEwen commented on pull request #13820: [FLINK-19671][codestyle] Revert .editorconfig change violating our coding style
Posted by GitBox <gi...@apache.org>.
StephanEwen commented on pull request #13820:
URL: https://github.com/apache/flink/pull/13820#issuecomment-717967578
Thanks for spotting this.
Is it also an option to adjust the written coding style?
I think that having the extra indentation for continuation helps to separate it from the general method body. I remember seeing this appreciated by many other devs during reviews.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org