Github user NicoK commented on the issue:
https://github.com/apache/flink/pull/4447
closed via c3235c395f7bb69b482b85c6832d427100130ca3
---
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4447
#4445 contains this PR, yes.
---
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4447
This seems to be subsumed by #4445 - is that correct?
---
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4447
Will merge it once travis gives a green light.
---
Github user NicoK commented on the issue:
https://github.com/apache/flink/pull/4447
PR updated by rebasing onto master and dropping the FLINK-7310 commits from
#4445.
---
Github user NicoK commented on the issue:
https://github.com/apache/flink/pull/4447
oh, yes - maybe I should rebase this one and exclude #4445 commits since
there, we may actually investigate further
---
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4447
Doesn't this PR still depend on #4445? Thats why i didn't merge it so far.
---
Github user NicoK commented on the issue:
https://github.com/apache/flink/pull/4447
Just curious whether we can merge this now - it has been laying around for
too long.
---
Github user NicoK commented on the issue:
https://github.com/apache/flink/pull/4447
FYI: I just rebased this PR onto current `master` to make this mergable and
support further extensions
---
If your project is set up for it, you can reply to this email and have your
reply appear on G
Github user NicoK commented on the issue:
https://github.com/apache/flink/pull/4447
:) in here, however, I had to add an exception for these `final` keywords
which may be removed when #4458 is merged.
---
If your project is set up for it, you can reply to this email and have your
rep
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4447
oh, you already did. Well, never mind. ;)
---
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
ena
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4447
@NicoK could you revert the removal of `final` and rebase the PR?
---
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 n
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4447
see #4458 .
---
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
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4447
I'll go through the commits tomorrow and revert the removals, and exclude
methods from the `RedundantModifier` rule.
---
If your project is set up for it, you can reply to this email and have your
re
Github user greghogan commented on the issue:
https://github.com/apache/flink/pull/4447
@StephanEwen we can search the diffs for commits with `checkstyle` in the
summary for removal of `final`, which only yields ~100 results with only a
handful requiring reversion.
---
If your proje
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4447
Final key-words on non-static methods should always be an exception, no
matter whether checkstyle marks them as a violation.
Talking to @NicoK offline, he mentioned that there have been c
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4447
For now I'd rather stick to adding suppressions for the files where we deem
it important, which is easy to do btw. If the resulting set of suppressions is
small let's stick with it, and revisit the di
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4447
Thinking more about this, I think we should modify the checkstyle to not
force us to remove such `final` keywords. While being redundant in the current
"snapshot" of the code, they may not be red
Github user greghogan commented on the issue:
https://github.com/apache/flink/pull/4447
@NicoK, just wanted to suggest that (other than trivial or single file
modifications) to have separate commits for trailing whitespace and import
order, then one or more checkstyle commits as neces
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4447
If the tooling is hard to be made to cooperate, then I am not religious
about the `final` keyword here.
This is more of a general theme: I am trying to advocate to not change
things "jus
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4447
We could however suppress the `RedundantModifier` check for the
`HeapMemorySegment` class.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as we
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4447
FYI, there's no way to configure checkstyle to ignore the final keyword on
methods of final classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on Gi
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4447
Great, if reviewer time is the only concern we can go ahead with #4446, I
don't mind spending some free time on it.
---
If your project is set up for it, you can reply to this email and have your
rep
Github user NicoK commented on the issue:
https://github.com/apache/flink/pull/4447
FYI: the `final` keyword on methods in `final` classes is something that is
actually forbidden by the checkstyle, hence the change
Regarding the try-catch in tests (actually part of #4446): you
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4447
There is also a complete reorg of a test file, removing try/catch blocks.
This affects to my knowledge only the way where exactly the stack trace is
printed.
I am skeptical about these t
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4447
At the first quick glance: This is removing a lot of `final` keywords from
various methods.
While one could argue that this keyword is not strictly necessary (the
class as a whole is fin
26 matches
Mail list logo
