Re: Please use ReviewBoard

[Vlas Puhov]

On Saturday 27 July 2013, 14:07 +02:00 Milian Wolff <mail at milianw.de> wrote:
>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.
H'm...I didn't think about it. It's a good idea. Thanks.
>
>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)
Nope, it's not avaliable, not anymore. This as you call it "the big hunk" was wrong from the start. If you take a close look you'll notice that this hunk didn't let you search in all opened projects, instead it opened just a first one. Futhermore there was the stupid check: first !m_directory.isEmpty() was checked and then if it's empty: !m_directory.startsWith(proj->folder().toLocalFile(). What a hell is that??? How can it start with something if it's empty?

>
>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.
 I don't know if it really needs a comment. I mean, there is only only two lines of code. I've seen hunderds lines of code in kdevelop's code base without even a single word of explanation about what it does. So, I think it's ok.
And what is wrong with ternary operator? 
>
>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
At first I thought about it too. But there is a little problem: BreakpointWidget is not the only one who retrieves data by DisplayRole. Actually it's the right thing for breakpoint model to return the full path instead of a file's name(it's more useful, for example breakpoint insertion uses the full path).
>
>
>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     
THANK YOU!!! Really. I didn't know about it.
>
>
>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
>
PS: I'd love to use the reviewboard. But for me it looks like there is no one who is proficient enough in code base and at the same time interested in reviewing patches. For example,  I posted a couple of patches : 1. Fix crash in disassembleWidged. It was on the reviewboard for 2 weeks - not a single comment. 2. Fix incorect debug session ending - 2 weeks too. Yeah Aleix wrote something,  but he didn't tell(no one did!) that I was patching in a wrong place. 3. Fix debug/execute actions avaliable while none of projets are opened - was for review about 2 weeks too. Yes I know I was completely wrong about it, but afterwards I wasn't disabling those actions anymore, I just speeded a project's loading up, but still no comments. 
  So, the question is why? What is the meaning in posting patches on reviewboard if there is no one to review it?

-- 
Vlas Puhov

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20130727/18dfa608/attachment.html>