Re: [DISCUSS] Code style / checkstyle

Cool! So, it begins =) - Henry On Wed, Apr 26, 2017 at 7:30 AM, Aljoscha Krettek wrote: > I merged the stricter checkstyle for flink-streaming-java today. (Sans > checking for right curly braces) > > > On 18. Apr 2017, at 22:17, Chesnay Schepler wrote:

Re: [DISCUSS] Code style / checkstyle

I merged the stricter checkstyle for flink-streaming-java today. (Sans checking for right curly braces) > On 18. Apr 2017, at 22:17, Chesnay Schepler wrote: > > +1 to use earth rotation as the new standard time unit. Maybe more > importantly, I'm absolutely in favor of

Re: [DISCUSS] Code style / checkstyle

+1 to use earth rotation as the new standard time unit. Maybe more importantly, I'm absolutely in favor of merging it. On 18.04.2017 20:39, Aljoscha Krettek wrote: I rebased the PR [1] on current master. Is there any strong objection against merging this (minus the two last commits which

Re: [DISCUSS] Code style / checkstyle

I rebased the PR [1] on current master. Is there any strong objection against merging this (minus the two last commits which introduce curly-brace-style checking). If not, I would like to merge this after two earth rotations, i.e. after all the time zones have had some time to react. The

Re: [DISCUSS] Code style / checkstyle

I agree to just allow both. While I definitely prefer 1) i can see why someone might prefer 2). Wouldn't want to delay this anymore; can't find to add this to flink-metrics and flink-python... On 03.04.2017 18:33, Aljoscha Krettek wrote: I think enough people did already look at the

Re: [DISCUSS] Code style / checkstyle

Could we use Aljoscha’s proposed checkstyle [0] as a baseline for discussion? It does not define a line length and we could bid down from there. Chesnay not only reviewed the changes but summarized the checkstyle [1]: It enforces the following rules: • every file has to end with a

Re: [DISCUSS] Code style / checkstyle

I think enough people did already look at the checkstyle rules proposed in the PR. On most of the rules reaching consensus is easy so I propose to enable all rules except those regarding placement of curly braces and control flow formatting. The two styles in the Flink code base are: 1) if

Re: [DISCUSS] Code style / checkstyle

I played around with this over the week end and it turns out that the required changes in flink-streaming-java are not that big. I created a PR with a proposed checkstyle.xml and the required code changes: https://github.com/apache/flink/pull/3567 .

Re: [DISCUSS] Code style / checkstyle

I added an issue for adding a custom checkstyle.xml for flink-streaming-java so that we can gradually add checks: https://issues.apache.org/jira/browse/FLINK-6107. I outlined the procedure in the Jira. We can use this as a pilot project and see how it goes and then gradually also apply to

Re: [DISCUSS] Code style / checkstyle

A singular "all reformat in one instant" will cause immense damage to the project, in my opinion. - There are so many pull requests that we are having a hard time keeping up, and merging will a lot more time intensive. - I personally have many forked branches with WIP features that will

Re: [DISCUSS] Code style / checkstyle

It is actually Databricks Scala guide to help contributions to Apache Spark so not really official Spark Scala guide. The style guide feels bit more like asking people to write Scala in Java mode so I am -1 to follow the style for Apache Flink Scala if that what you are recommending. If the

Re: [DISCUSS] Code style / checkstyle

By the way, I also don't see the benefit of doing the transition piece by piece. On Mon, 27 Feb 2017 at 22:21 Dawid Wysakowicz wrote: > I agree with adopting a custom codestyle/checkstyle for flink, but as I > understood correctly most people agree there is no point

Re: [DISCUSS] Code style / checkstyle

I agree with adopting a custom codestyle/checkstyle for flink, but as I understood correctly most people agree there is no point of providing an unenforced code style. 2017-02-27 22:04 GMT+01:00 Greg Hogan : > There was also … > > - create a flink style (for example, leaving

Re: [DISCUSS] Code style / checkstyle

There was also … - create a flink style (for example, leaving indentation as +1 tab rather than +2 spaces as in google's style) - create a flink style but optional and unenforced (but recommended for new contributions) Flink currently has a reasonably consistent code style. I expect that

Re: [DISCUSS] Code style / checkstyle

So to sum up all the comments so far we have two alternatives. We either: 1) introduce unified checkstyle (with enforcing) and corresponding code style, both based on some established ones like google code style for java [1] and scalastyle for scala

Re: [DISCUSS] Code style / checkstyle

+1 to provide and enforcing a unified code style for both java and scala. Unification should apply when it makes sense like comments though. Eventually code base should be re-factored. I would vote for the one at a time module fix apporoach. Style guide should be part of any PR review. We could

Re: [DISCUSS] Code style / checkstyle

I agree, reformatting 90% of the code base is tough. There are two main issues: (1) Incompatible merges. This is hard, especially for the folks that have to merge the pull requests ;-) (2) Author history: This is less of an issue, I think. "git log " and "git show -- " will still work and

Re: [DISCUSS] Code style / checkstyle

Just for a bit of context, this is the output of running cloc on the Flink codebase: --- Language files blankcomment code

Re: [DISCUSS] Code style / checkstyle

No, I think that's exactly what people mean when saying "losing the commit history". With the reformatting you would have to go manually through all past commits until you find the commit which changed a given line before the reformatting. Cheers, Till On Sun, Feb 26, 2017 at 6:32 PM, Alexander

Re: [DISCUSS] Code style / checkstyle

Just want to clarify what unify code style here. Is the intention to have IDE and Maven plugins to have the same check style rules? Or are we talking about having ONE code style for both Java and Scala? - Henry On Fri, Feb 24, 2017 at 8:08 AM, Greg Hogan wrote: > I agree

Re: [DISCUSS] Code style / checkstyle

I agree wholeheartedly with Ufuk. We cannot reformat the codebase, cannot pause while flushing the PR queue, and won't find a consensus code style. I think we can create a baseline code style for new and existing contributors for which reformatting on changed files will be acceptable for PR

Re: [DISCUSS] Code style / checkstyle

The problem with code style when it is not enforced is that it will be a matter of luck to what parts of files / new files will it be applied. When the code style is not applied to whole file, it is pretty much useless anyway. You would need to manually select just the fragments one is changing.

Re: [DISCUSS] Code style / checkstyle

On Fri, Feb 24, 2017 at 10:46 AM, Fabian Hueske wrote: > I agree with Till that encouraging a code style without enforcing it does > not make a lot of sense. > If we enforce it, we need to touch all files and PRs. I think it makes sense for new contributors to have a starting

Re: [DISCUSS] Code style / checkstyle

We have discussed this issue several times in the past and never got around to do it. Although I agree that it would be nice to have a stricter code style, I don't think we should introduce a code style. IMO, at this point the costs (losing the history, PR hassle, etc.) would be too high compared

Re: [DISCUSS] Code style / checkstyle

Thanks to discuss this problem again. 1. Google checkstyle is good for java. 2. scala check style is here [1] 3. We can make a Flink plan contain issues, one sub-issue one rule. Resolve this in short time. Current code style may be historical accumulate. If we don’t normalize the code step by

Re: [DISCUSS] Code style / checkstyle

If we go for a codestyle/checkstyle I would suggest to use the Google style. This already has checkstyle, IntelliJ style, Eclipse style and a code formatting tool and is well established. However, some people will not like this style. In general, I think we will never manage to find a style that

Re: [DISCUSS] Code style / checkstyle

So how about preparing a code style and corresponding checkstyle and enabling it gradually module by module whenever shepherd/commiter with expertise in a module will have time to introduce/check such a change? Maybe it will make the "snowball effect" happen? I agree there is no point in preparing

Re: [DISCUSS] Code style / checkstyle

For file where we don't enforce checkstyle things should work they way they do right now. Turn off auto-formatting, and only format code that you touched and that's it. For these modification we will have to check them manually in the PRs as we do now. On 22.02.2017 16:22, Greg Hogan wrote:

Re: [DISCUSS] Code style / checkstyle

Will not the code style be applied on save to any user-modified file? So this will clutter PRs and overwrite history. On Wed, Feb 22, 2017 at 6:19 AM, Dawid Wysakowicz < wysakowicz.da...@gmail.com> wrote: > I also agree with Till and Chesnayl. Anyway as to "capture the current > style" I have

Re: [DISCUSS] Code style / checkstyle

I agree with Till. I would propose enforcing checkstyle on a subset of the modules, basically those that are not flink-runtime, flink-java, flink-streaming-java. These are the ones imo where messing with the history can be detrimental; for the others it isn't really important imo. (Note that

Re: [DISCUSS] Code style / checkstyle

I think that not enforcing a code style is as good as not having any code style to be honest. Having an IntelliJ or Eclipse profile is nice and some people will probably use it, but my gut feeling is that the majority won't notice it. Cheers, Till On Wed, Feb 22, 2017 at 11:15 AM, Ufuk Celebi

Re: [DISCUSS] Code style / checkstyle

Kurt's proposal sounds reasonable. What about the following: - We try to capture the current style in an code style configuration (for IntelliJ and maybe Eclipse) - We provide that on the website for contributors to download - We don't enforce it, but new contributions and changes are free to

[DISCUSS] Code style / checkstyle

Hi, I would like to resurrect the discussing ([1] , [2] ) about creating unified code