Re: PyKF5 bindings generator improvements out for review.
Shaheed Haque <[email protected]>
| Newsgroups | gmane.comp.kde.devel.bindings |
|---|---|
| Message-ID | <CAHAc2jcQ8QVk_LvjMX+gkJ78PQu8Lv=TQdfMPN_r_ujJYdGv+Q@mail.gmail.com> |
Rewrite? It think its more a case of forking making things diverge (if you look, a lot of the changes are formatting, name changes and comments, though I accept that does make for a lot of noise). I have tried VERY hard to keep the good changes form both sides...and yes, there ARE a reasonable number of actual changes too. Anyway, I was under the impression a squashed single commit was preferred, but I can look to pull things out again. I would prefer to work the reviews via github's Pull Requests, is that still OK? On 29 January 2017 at 16:24, Stephen Kelly <[email protected]> wrote: > Shaheed Haque wrote: > > > All the code is squashed into the single commit. Feedback welcome here or > > via reviewboard or via github. > > Hi Shaheed, > > Having so many changes in a single commit is not really reviewable now or > in > the future. Commits should be minimal so that people in the future who > examine the history can make sense of it. The commit message for that > commit > says > > Re-introduce the missing databases. > > but in fact it appears to be a complete re-write in a single commit. > > Please extract meaningful changes into separate commits. For example, the > change to > > find-modules/Qt5Ruleset.py > > must correspond to only a part of the rest of the commit. Please extract > that into a commit. Then consider any other changes in the big commit and > extract small meaningful commits and repeat. > > When reviewing commits the reader should have only one context in mind. For > example "ok, this commit is changing the API of rulesets" or "ok, this > commit is introducing a noop so enable silencing warnings". > > Usually that context for the commit should be in the commit message! > > Thanks, > > Steve. >