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