Vojtech Szocs has posted comments on this change. Change subject: webadmin: removed caching from EnumTranslator. ......................................................................
Patch Set 3: > Question 1: What's the definition of "related changes"? If I have to step > into badly formatted > method or overgrown method or what ever, can I fix it or should I do it as a > separate commit on > which my commit depends? If I separate it into smaller commit (which I would > like to do) I'll be > dependant on those commit approval which can take long time. And neglecting > to fix evident errors > just beacause don't want to be related to those fixes or aren't relevant to > mine changes stop code > improvements. So what's the corrent way out of this 3? Same commit / another > commit / do not commit > &deal with it. That's a very good question, the answer usually varies among different developers. Let's say you're writing a patch that could modify following methods of a given class: * method a() modified because it's needed to address the issue at hand (i.e. solve problem described in BZ ticket) * method b() modified because of its form: bad formatting, not following proper code conventions, hard-to-read code, etc. * method c() modified because of its function: inefficient/unnecessarily-complex code, code that leaks memory, etc. Most commonly, only method a(), which is directly related to the issue at hand, should be modified in given patch that solves that issue. This is to make it easier to review the essence of the change and to show courtesy to reviewers who must review all changes and think about possible implications. At the same time, we should never forget about methods b() and c() - these should be modified in a separate patch. So we end up with two patches, first one containing essential changes, second one containing non-essential (but still relevant) changes. I recommend rebasing non-essential patch on top of essential one (and not the other way around). -- To view, visit http://gerrit.ovirt.org/26048 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3df424d8e7a35ed249b2760da79deba7db31b785 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
