Following up on my previous email: Every blueprint specification has a Work items section. That section should describe granular work items, not just things in general. We should encourage components leads to put their attention to this aspect.
> 7 груд. 2015 р. о 12:23 Roman Prykhodchenko <m...@romcheg.me> написав(ла): > > Maciej, thanks for bringing this topic up for the discussion! > > I absolutely agree with the idea that at this point we have a problem with > patch size. Patches that are too big usually have two major issues: they > either don’t get reviewed for a very long time or get random +1s from people > who don’t even bother reading all the changes there. > > Splitting a patch when it’s already published may be pretty problematic, so > we should think about granularity of every feature on the design stage. Only > in that way we will be able to submit small patches w/o making compromises on > test coverage, stability or any other important aspect of the code quality. > > Folks proposed two different solutions here: > 1. Set a CI job that puts a -1 to every patch that’s bigger than a certain > limit. > 2. Make reviewers responsible for -1ing a patch. > > Most of the patches that are bigger than 500 LOC can be split. However, there > is a lot of corner cases we have to carry about, if we are going to put a -1 > to all of them automatically. In my opinion we should not do that. Instead, > we can send warning emails to core teams if this kind of patch set is > submitted. Then a core team may easily analyze whether this patch can be > split and propose a meaningful way to do that. > > Huge patches cause a different story — they are incredibly hard for making a > good review, merging or reverting them can lead to a major regression in a > lot of places, they cause conflicts for a lot of other patches and so on. A > huge patch ALWAYS means exclusively one of the following: > 1. It can be split > 2. The design of the feature is completely wrong. > > We need to set a treshold for a huge patch and block all patches that exceed > it by putting -2 scores. > > Summary: > > - Let’s set two thresholds — one for to big and one for huge patches. > - Let’s create a job that will warn an appropriate core team if a too big > patch is submitted and block huge patches. > > > - romcheg > > > >> 7 груд. 2015 р. о 11:23 Stanislaw Bogatkin <sbogat...@mirantis.com >> <mailto:sbogat...@mirantis.com>> написав(ла): >> >> > What if you're not sure how the improved code should look like, but >> > you definitely sure that it shouldn't look like proposed one? :) >> >> I believe you should ask other people if you are right, as set '-1' to code >> that you >> cannot improve is not the best option, so >> >> >> > If you are not sure how the improved code would look like, just let it go! >> is right >> >> >> Also I think that set some strict boundaries, like 400 LOC per patch is >> wrong. For example, if you >> introduce new test fixture for fuel-library, it usually about 800+ LOC and >> you can't split it >> out, so we should either move such fixtures out of code or make an exeption >> for such type of code. >> >> On Mon, Dec 7, 2015 at 1:03 PM, Igor Kalnitsky <ikalnit...@mirantis.com >> <mailto:ikalnit...@mirantis.com>> wrote: >> Hey Andrii, >> >> I'm totally agree with you about contribution guides, except one thing >> - we need and should force users to split huge patches into set of >> small ones. Mostly because it will simplify support and review of >> patches. Previously we ignored this rule pretty often, and get a lot >> of troubles due to buggy code. >> >> Also, see some comments below. >> >> > So, when an author of a patch gets -1 with the statement «split this >> > code», I believe it is not constructive. At least you should roughly >> > describe how you see it, how the patch could be split, you should be >> > helpful to the author of a patch. >> >> No one is suggesting to set -1 without leaving a comment how the patch >> could be divided. >> >> >> > If you are not sure how the improved code would look like, just let it go! >> >> What if you're not sure how the improved code should look like, but >> you definitely sure that it shouldn't look like proposed one? :) >> >> I'd say it's a good reason to set -1 and start discussion. Not only >> with author, but with other folks, since everyone in community is >> responsible of code quality of the project. >> >> - I >> >> On Mon, Dec 7, 2015 at 3:28 AM, Andrey Tykhonov <atykho...@mirantis.com >> <mailto:atykho...@mirantis.com>> wrote: >> > I believe this is against the code review guidelines. >> > >> > «Comments must be meaningful and should help an author to change the >> > code the right way.» [1] >> > >> > If you get a comment that says «split this change into the smaller >> > commit» I'm sorry, but it doesn't help at all. >> > >> > «Leave constructive comments >> > >> > Not everyone in the community is a native English speaker, so make >> > sure your remarks are meaningful and helpful for the patch author to >> > change his code, but also polite and respectful. >> > >> > The review is not really about the score. It's all about the >> > comments. When you are reviewing code, always make sure that your >> > comments are useful and helpful to the author of the patch. Try to >> > avoid leaving comments just to show that you reviewed something if >> > they don't really add anything meaningful» [2] >> > >> > So, when an author of a patch gets -1 with the statement «split this >> > code», I believe it is not constructive. At least you should roughly >> > describe how you see it, how the patch could be split, you should be >> > helpful to the author of a patch. So, first of all, you need to review >> > the patch! :) >> > >> > I want to emphasize this: «The review is not really about the >> > score. It's all about the comments.» >> > >> > «In almost all cases, a negative review should be accompanied by clear >> > instructions for the submitter how they might fix the patch.» [4] >> > >> > I believe that the statement "split this change into the smaller >> > commit" is too generic, it is mostly the same as the "this patch needs >> > further work". It doesn't bring any additional instructions how >> > exactly a patch could be fixed. >> > >> > Please also take a loot at the following conversation from mailing >> > list: [3]. >> > >> > «It's not so easy to guess what you mean, and in case of a clumsy >> > piece of code, not even that certain that better code can be used >> > instead. So always provide an example of what you would rather want to >> > see. So instead of pointing to indentation rules, just show properly >> > indented code. Instead of talking about grammar or spelling, just type >> > the corrected comment or docstring. Finally, instead of saying "use >> > list comprehension here" or "don't use has_key", just type your >> > proposal of how the code should look like. Then we have something >> > concrete to talk about. Of course, you can also say why you think this >> > is better, but an example is very important. If you are not sure how >> > the improved code would look like, just let it go, chances are it >> > would look even worse.» [3] >> > >> > So, please, bring something concrete to talk about. If you are not >> > sure how the improved code would look like, just let it go! >> > >> > «The simplest way to talk about code is to just show the code. When you >> > want the author to fix something, rewrite it in a different way, >> > format the code differently, etc. -- it's best to just write in the >> > comment how you want that code to look like. It's much faster than >> > having the author guess what you meant in your descriptions, and also >> > lets us learn much faster by seeing examples.» [2] >> > >> > >> > [1] >> > https://docs.google.com/document/d/1tyKhHQRQqTEW6tS7_LCajEpzqn55f-f5nDmtzIeJ2uY/edit >> > >> > <https://docs.google.com/document/d/1tyKhHQRQqTEW6tS7_LCajEpzqn55f-f5nDmtzIeJ2uY/edit> >> > [2] https://wiki.openstack.org/wiki/CodeReviewGuidelines >> > <https://wiki.openstack.org/wiki/CodeReviewGuidelines> >> > [3] >> > http://www.mail-archive.com/openstack-dev@lists.openstack.org/msg07831.html >> > >> > <http://www.mail-archive.com/openstack-dev@lists.openstack.org/msg07831.html> >> > [4] http://docs.openstack.org/infra/manual/developers.html#peer-review >> > <http://docs.openstack.org/infra/manual/developers.html#peer-review> >> > >> > >> > Best regards, >> > Andrey Tykhonov (tkhno) >> > >> > >> > __________________________________________________________________________ >> > OpenStack Development Mailing List (not for usage questions) >> > Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe >> > <http://openstack-dev-requ...@lists.openstack.org/?subject:unsubscribe> >> > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >> > <http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev> >> > >> >> __________________________________________________________________________ >> OpenStack Development Mailing List (not for usage questions) >> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe >> <http://openstack-dev-requ...@lists.openstack.org/?subject:unsubscribe> >> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >> <http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev> >> >> __________________________________________________________________________ >> OpenStack Development Mailing List (not for usage questions) >> Unsubscribe: openstack-dev-requ...@lists.openstack.org >> <mailto:openstack-dev-requ...@lists.openstack.org>?subject:unsubscribe >> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >> <http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev> >
signature.asc
Description: Message signed with OpenPGP using GPGMail
__________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
