I would like to extend the coding guidelines to make new tests (or extending existing once) for fixed bugs mandatory; ie, write a test that fails before the fix, but passes after the fix.
Right now, the guideline dictates that **Tests for new features are required** which is not broad enough, IMHO. What do you think? -Matthias On 10/05/2015 02:09 PM, Fabian Hueske wrote: > Hi, > > I opened a PR with the discussed changes [1]. > Please review, give feedback, and suggest changes. > > Cheers, Fabian > > [1] https://github.com/apache/flink-web/pull/11 > > > 2015-09-28 18:02 GMT+02:00 Fabian Hueske <fhue...@gmail.com>: > >> @Chiwan, sure. Will do that. Thanks for pointing it out :-) >> >> 2015-09-28 18:00 GMT+02:00 Chiwan Park <chiwanp...@apache.org>: >> >>> @Fabian, Could you cover FLINK-2712 in your pull request? I think that it >>> would be better than split pull request. >>> >>> Regards, >>> Chiwan Park >>> >>>> On Sep 28, 2015, at 4:51 PM, Fabian Hueske <fhue...@gmail.com> wrote: >>>> >>>> Thanks everybody for the discussion. >>>> I'll prepare a pull request to update the "How to contribute" and >>> "Coding >>>> Guidelines". >>>> >>>> Thanks, >>>> Fabian >>>> >>>> 2015-09-26 9:06 GMT+02:00 Maximilian Michels <m...@apache.org>: >>>> >>>>> Hi Fabian, >>>>> >>>>> This is a very important topic. Thanks for starting the discussion. >>>>> >>>>> 1) JIRA discussion >>>>> >>>>> Absolutely. No new feature should be introduced without a discussion. >>>>> Frankly, I see the problem that sometimes discussions only come up >>>>> when the pull request has been opened. However, this can be overcome >>>>> by the design document. >>>>> >>>>> 2) Design document >>>>> >>>>> +1 for the document. It increases transparency but also helps the >>>>> contributor to think his idea through before starting to code. The >>>>> document could also be written directly in JIRA. That way, it is more >>>>> accessible. JIRA offers mark up; even images can be attached and >>>>> displayed in the JIRA description. >>>>> >>>>> I'd like to propose another section "Limitations" for the design >>>>> document. Breaking API changes should also be listed on a special Wiki >>>>> page. >>>>> >>>>> 3) Coding style >>>>> >>>>> In addition to updating the document, do we want to enforce coding >>>>> styles also by adding new Maven Checkstyle rules? IMHO strict rules >>>>> could cause more annoyances than they actually contribute to the >>>>> readability of the code. Perhaps this should be discussed in a >>>>> separate thread. >>>>> >>>>> +1 for collecting common problems and design patterns to include them >>>>> in the document. I was thinking, that we should also cover some of the >>>>> features of tools and dependencies we heavily use, e.g. Travis, >>>>> Mockito, Guava, Log4j, FlinkMiniCluster, Unit testing vs IT cases, >>>>> etc. >>>>> >>>>> 4 ) Restructuring the how to contribute guide >>>>> >>>>> Good idea to have a meta document that explains how contributing works >>>>> in general, and another document for technical things. >>>>> >>>>> >>>>> Cheers, >>>>> Max >>>>> >>>>> >>>>> On Thu, Sep 24, 2015 at 2:53 PM, Fabian Hueske <fhue...@gmail.com> >>> wrote: >>>>>> >>>>>> Thanks everybody for feedback and comments. >>>>>> >>>>>> Regarding 1) and 2): >>>>>> >>>>>> I like the idea of keeping the discussion of new features and >>>>> improvements >>>>>> in JIRA as Kostas proposed. >>>>>> Our coding guidelines [1] already request a JIRA issue for each pull >>>>>> request. >>>>>> >>>>>> How about we highlight this requirement more prominently and follow >>> this >>>>>> rule more strict from now on. >>>>>> JIRA issues for new features and improvements should clearly specify >>> the >>>>>> scope and requirements for the new feature / improvement. >>>>>> The level of detail is up to the reporter of the issue, but the >>> community >>>>>> can request more detail or change the scope and requirements by >>>>> discussion. >>>>>> When a JIRA issue for a new feature or improvement is opened, the >>>>> community >>>>>> can start a discussion whether the feature is desirable for Flink or >>> not. >>>>>> Any contributor (including the reporter) can also attach a >>>>>> "design-doc-requested" label to the issue. A design document can be >>>>>> proposed by anybody, including the reporter or assignee of the JIRA >>>>> issue. >>>>>> However, the issue cannot be resolved and a corresponding PR not be >>>>> merged >>>>>> before a design document has been accepted by lazy consensus. Hence, >>> an >>>>>> assignee should propose a design doc before starting to code to avoid >>>>> major >>>>>> redesigns of the implementation. >>>>>> >>>>>> This way it is up to the community when to start a discussion about >>>>> whether >>>>>> a feature request is accepted or to request a design document. We can >>>>> make >>>>>> design documents mandatory for changes that touch the public API. >>>>>> >>>>>> Regarding 3): >>>>>> >>>>>> I agree with Vasia, that we should collect suggestions for common >>>>> patterns >>>>>> and also continuously update the coding guidelines. >>>>>> @Henry, I had best practices (exception handling, tests, etc.) in >>> mind. >>>>>> Syntactic code style is important as well, but we should have a >>> separate >>>>>> discussion about that, IMO. >>>>>> >>>>>> Proposal for a design document template: >>>>>> >>>>>> - Overview of general approach >>>>>> - API changes (changed interfaces, new / deprecated configuration >>>>>> parameters, changed behavior) >>>>>> - Main components and classes to touch >>>>>> >>>>>> Cheers, >>>>>> Fabian >>>>>> >>>>>> [1] http://flink.apache.org/coding-guidelines.html >>>>>> <http://flink.apache.org/coding-guidelines.html> >>>>>> >>>>>> 2015-09-24 10:52 GMT+02:00 Chiwan Park <chiwanp...@apache.org>: >>>>>> >>>>>>> Thanks Fabian for starting the discussion. >>>>>>> >>>>>>> +1 for overall approach. >>>>>>> >>>>>>> About (1), expressing that consensus must be required for new feature >>>>> in >>>>>>> “How to contribute” page is very nice. Some pull requests were sent >>>>> without >>>>>>> consensus. The contributors had to rewrote their pull requests. >>>>>>> >>>>>>> Agree with (2), (3) and (4). >>>>>>> >>>>>>> Regards, >>>>>>> Chiwan Park >>>>>>> >>>>>>>> On Sep 24, 2015, at 2:23 AM, Henry Saputra <henry.sapu...@gmail.com >>>> >>>>>>> wrote: >>>>>>>> >>>>>>>> Thanks again, Fabian for starting the discussions. >>>>>>>> >>>>>>>> For (1) and (2) I think it is good idea and will help people to >>>>>>>> understand and follow the author thought process. >>>>>>>> Following up with Stephan's reply, some new features solutions could >>>>>>>> be explained thoroughly in the PR descriptions but some requires >>>>>>>> additional reviews of the proposed design. >>>>>>>> I like the idea of using tag in JIRA whether new features should or >>>>>>>> should not being accompanied by design document. >>>>>>>> >>>>>>>> Agree with (3) and (4). >>>>>>>> As for (3) are you thinking about more of style of code syntax via >>>>>>>> checkstyle updates, or best practices in term of no mutable state if >>>>>>>> possible, throw precise Exception if possible for interfaces, etc. ? >>>>>>>> >>>>>>>> - Henry >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Wed, Sep 23, 2015 at 9:31 AM, Stephan Ewen <se...@apache.org> >>>>> wrote: >>>>>>>>> Thanks, Fabian for driving this! >>>>>>>>> >>>>>>>>> I agree with your points. >>>>>>>>> >>>>>>>>> Concerning Vasia's comment to not raise the bar too high: >>>>>>>>> That is true, the requirements should be reasonable. We can >>>>> definitely >>>>>>> tag >>>>>>>>> issues as "simple" which means they do not require a design >>>>> document. >>>>>>> That >>>>>>>>> should be more for new features and needs not be very detailed. >>>>>>>>> >>>>>>>>> We could also make the inverse, meaning we explicitly tag certain >>>>>>> issues as >>>>>>>>> "requires design document". >>>>>>>>> >>>>>>>>> Greetings, >>>>>>>>> Stephan >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Wed, Sep 23, 2015 at 5:05 PM, Vasiliki Kalavri < >>>>>>> vasilikikala...@gmail.com >>>>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> I agree with you Fabian. Clarifying these issues in the "How to >>>>>>> Contribute" >>>>>>>>>> guide will save lots of time both to reviewers and contributors. >>>>> It is >>>>>>> a >>>>>>>>>> really disappointing situation when someone spends time >>>>> implementing >>>>>>>>>> something and their PR ends up being rejected because either the >>>>>>> feature >>>>>>>>>> was not needed or the implementation details were never agreed on. >>>>>>>>>> >>>>>>>>>> That said, I think we should also make sure that we don't raise >>> the >>>>>>> bar too >>>>>>>>>> high for simple contributions. >>>>>>>>>> >>>>>>>>>> Regarding (1) and (2), I think we should clarify what kind of >>>>>>>>>> additions/changes require this process to be followed. e.g. do we >>>>> need >>>>>>> to >>>>>>>>>> discuss additions for which JIRAs already exist? Ideas described >>>>> in the >>>>>>>>>> roadmaps? Adding a new algorithm to Gelly/Flink-ML? >>>>>>>>>> >>>>>>>>>> Regarding (3), maybe we can all suggest some examples/patterns >>> that >>>>>>> we've >>>>>>>>>> seen when reviewing PRs and then choose the most common (or all). >>>>>>>>>> >>>>>>>>>> (4) sounds good to me. >>>>>>>>>> >>>>>>>>>> Cheers, >>>>>>>>>> Vasia. >>>>>>>>>> >>>>>>>>>> On 23 September 2015 at 15:08, Kostas Tzoumas < >>> ktzou...@apache.org >>>>>> >>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> Big +1. >>>>>>>>>>> >>>>>>>>>>> For (1), a discussion in JIRA would also be an option IMO >>>>>>>>>>> >>>>>>>>>>> For (2), let us come up with few examples on what constitutes a >>>>>>> feature >>>>>>>>>>> that needs a design doc, and what should be in the doc (IMO >>>>>>>>>>> architecture/general approach, components touched, interfaces >>>>> changed) >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Wed, Sep 23, 2015 at 2:24 PM, Fabian Hueske < >>> fhue...@gmail.com >>>>>> >>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>>> Hi everybody, >>>>>>>>>>>> >>>>>>>>>>>> I guess we all have noticed that the Flink community is quickly >>>>>>> growing >>>>>>>>>>> and >>>>>>>>>>>> more and more contributions are coming in. Recently, a few >>>>>>>>>> contributions >>>>>>>>>>>> proposed new features without being discussed on the mailing >>>>> list. >>>>>>> Some >>>>>>>>>>> of >>>>>>>>>>>> these contributions were not accepted in the end. In other >>> cases, >>>>>>> pull >>>>>>>>>>>> requests had to be heavily reworked because the approach taken >>>>> was >>>>>>> not >>>>>>>>>>> the >>>>>>>>>>>> best one. These are situations which should be avoided because >>>>> both >>>>>>> the >>>>>>>>>>>> contributor as well as the person who reviewed the contribution >>>>>>>>>> invested >>>>>>>>>>> a >>>>>>>>>>>> lot of time for nothing. >>>>>>>>>>>> >>>>>>>>>>>> I had a look at our “How to contribute” and “Coding guideline” >>>>> pages >>>>>>>>>> and >>>>>>>>>>>> think, we can improve them. I see basically two issues: >>>>>>>>>>>> >>>>>>>>>>>> 1. The documents do not explain how to propose and discuss new >>>>>>>>>> features >>>>>>>>>>>> and improvements. >>>>>>>>>>>> 2. The documents are quite technical and the structure could be >>>>>>>>>>> improved, >>>>>>>>>>>> IMO. >>>>>>>>>>>> >>>>>>>>>>>> I would like to improve these pages and propose the following >>>>>>>>>> additions: >>>>>>>>>>>> >>>>>>>>>>>> 1. Request contributors and committers to start discussions on >>>>> the >>>>>>>>>>>> mailing list for new features. This discussion should help to >>>>> figure >>>>>>>>>> out >>>>>>>>>>>> whether such a new feature is a good fit for Flink and give >>> first >>>>>>>>>>> pointers >>>>>>>>>>>> for a design to implement it. >>>>>>>>>>>> 2. Require contributors and committers to write design >>>>> documents for >>>>>>>>>>> all >>>>>>>>>>>> new features and major improvements. These documents should be >>>>>>> attached >>>>>>>>>>> to >>>>>>>>>>>> a JIRA issue and follow a template which needs to be defined. >>>>>>>>>>>> 3. Extend the “Coding Style Guides” and add patterns that are >>>>>>>>>> commonly >>>>>>>>>>>> remarked in pull requests. >>>>>>>>>>>> 4. Restructure the current pages into three pages: a general >>>>> guide >>>>>>>>>> for >>>>>>>>>>>> contributions and two guides for how to contribute to code and >>>>>>> website >>>>>>>>>>> with >>>>>>>>>>>> all technical issues (repository, IDE setup, build system, etc.) >>>>>>>>>>>> >>>>>>>>>>>> Looking forward for your comments, >>>>>>>>>>>> Fabian >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>> >>> >>> >>> >>> >>> >>> >> >
signature.asc
Description: OpenPGP digital signature
