[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
Github user NicoK commented on the issue: https://github.com/apache/flink/pull/4447 closed via c3235c395f7bb69b482b85c6832d427100130ca3 ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
Github user zentol commented on the issue: https://github.com/apache/flink/pull/4447 #4445 contains this PR, yes. ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
Github user zentol commented on the issue: https://github.com/apache/flink/pull/4447 Will merge it once travis gives a green light. ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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 enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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 not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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 feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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 commits with such changes merged. What do we do about these now? Are we sure we did not void a statement that the authors put in place to the code to be future proof`? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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 discussion otherwise. We've already covered large parts of the code-based with the existing rules, I'd rather stick to it for the remainder, with exceptions where necessary, to increase consistency. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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 redundant in the future. I think there is a big upside of writing code in a "future proof" way. The modifiers in this class are a good example of that - the JIT friendlyness of the methods is so important that they should not be affected by future changes the class hierarchy. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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 "just because it looks better". Once you did your first set of late night debugging sessions and found that the bug was introduced by a "cleanup" change, you may know why I feel quite strongly about this... --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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're right, I was too quick in arguing that stack traces are not printed - whenever there is a `e.printStackTrace()`, it should work but at least when developing with IntelliJ, this printout is not clickable (during debugging a failure) compared to failing with a proper exception. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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 types of refactorings: - Either the reviewer does not really look at them, in which case the danger of accidentally weakening the test coverage by far outweigh the (in my opinion quite subjective and cosmetic) benefits of such a change. - Or the reviewer really checks the diff, in which case I would suggest to dedicate that time to other pull requests that bring in actual improvements. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #4447: [FLINK-7312][checkstyle] activate checkstyle for flink/co...
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 final), I put them there on purpose when writing this originally, to "document" the intention that these methods should not be ever overridden, regardless of whether the class as a whole would be subclass-able. I would vote to leave this in place, actually. While actually improving / cleaning up bad code is a nice thing to do as part of reworking parts, I think this is good practice to not try and just change things that could be changed, just for the sake of changing them, or because of a personal view of "what would look nicer". There is frequently a reason why things are as they are, especially in modules like the memory segments that were written very carefully and consciously. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
