ahmadsamir added inline comments. INLINE COMMENTS
> kossebau wrote in kkeysequencewidget.cpp:127 > Still unsure what you mean with the constness, as the QHash `shortcuts` has > been const all the time, and thus the access methods and its returned > references. If you meant the initially proposed `const QKeySequence &seq = > it.key();` that was just an alias reference to something const ref before > (`it.key()`), whose idea was to not touch the other existing code as well as > make it more obvious to the code reader what `it.key()` is. It did not change > any constness. > > Back to your original comment: > So here my 2 penny collected over decades: avoid that. One change/aspect at a > time. No additional clean-up., as basic as it is (even no whitespace changes, > unless line touched anyway). > > - The commit message might miss to mention that change, or make it more > complicated to read because it lists all the while-at-it changes. > - There are no obvious changes, unless documented. What is clear to the > commit creator, might be unclear to the commit reader, as they have another > context > - Line-wise commit annotation mark-up will be set for lines which are changed > while not relevant for the actual main change (which only would be mentioned > in the commit message first line/title) > - One is concentrated on the main change, and might miss important details > relevant to that other change, and introduce regressions. > > You may discard these 2 penny of mine, but let's talk again in some years ;) > Better though ask the search engine for what other people recommend as best > commit practice & compare. Still you are free to collect your own experience: > if young people only did what old people tell them, new discoveries would > never be made ;) But most of the times... I do try to keep commits atomic, and detail my changes in the commit message. (And no, I won't discard your 2 pennies, I am too poor, experience-wise, to afford that). What I should have done was read the code starting at the top, where shortcuts is declared const. Anyway thanks for explaining things, that's always appreciated. :) REPOSITORY R263 KXmlGui REVISION DETAIL https://phabricator.kde.org/D23813 To: kossebau, dfaure Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
