Please use ReviewBoard
Milian Wolff
mail at milianw.de
Sat Jul 27 12:07:21 UTC 2013
Hello Vlas,
while I greatly appreciate your enthusiasm I have to take on my maintainer hat
and ask you kindly to use ReviewBoard some more before pushing commits
directly to KDevelop codebase.
This commit for example needs to be revisited, I think:
commit fe61dcf2259bcd97d2e5649070cb38f11c4ec963
Author: Vlas Puhov <vlas.puhov at mail.ru>
Date: Fri Jul 26 21:14:34 2013 +0400
GrepViewPlugin: Search by default in previous location.
BUG: 299751
It should definitely use a combo box with a list of recent locations, similar
to the pattern history and other fields already do.
Furthermore, please add more information to your commit messages. This one
for example should state that the big hunk you removed is still "available" to
the user from within the dialog by using the synchronize button (at least, I
hope so).
This commit could also be cleaned up:
commit 722457052892e3a79f40f82f3f2e005abcd3f91d
Author: Vlas Puhov <vlas.puhov at mail.ru>
Date: Sat Jul 20 16:15:00 2013 +0400
BreakpointWidget: Show only file's name, the tooltip shows the full path.
BreakpointDelegate::displayText's implementation should not use the ternary
operator in my opinion and also needs a comment to clarify what it actually
does. Furthermore, please put opening braces of function bodies on their own
line, similar to how the other functions are doing it.
Even better though would be to remove the whole code there and put that logic
into the model, instead of inside the delegate. The code for that resides in
BreakPoint::data, where you need to special-case the DisplayRole then to only
return the filename. There it's also much simpler, as you have the url at hand
and can just use m_url.fileName() instead of string parsing.
Some patches you committed also contain whitespace errors, please enable
"remove trailing spaces on edited lines" in the editor settings. Also helpful
is something like this in your git config:
[core]
whitespace = trailing-space,space-before-tab
[apply]
whitespace = fix
Bye
PS: I hope you see this as constructive criticism. I really don't want to
annoy you or drive you away from the project - quite the contrary. I just hope
to teach you some more tricks to improve the quality of your contributions
even more. Using something like reviewboard is also how one learns a lot, by
taking in the suggestions from others.
Cheers! Rock on!
--
Milian Wolff
mail at milianw.de
http://milianw.de
More information about the KDevelop-devel
mailing list