Review Request: Exclude Comments in Find/Replace in Files

[Milian Wolff]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104230/#review11298
-----------------------------------------------------------


hey there, first up: what do you actually need this feature for? I can't really come up with cases where you look for code but want to exclude comments, if you are refactoring something, you want to replace the occurrences in comments as well.

then I see a big problem with such a feature, as it should be done properly if at all imo. Otherwise people will start to complain about $language not being supported properly etc. pp.

you should at least mention the supported languages in the UI somewhere. And of course, support these language properly which you do not do right now (see below).

please also write unit tests that verify the different cases for every supported languages


plugins/grepview/grepjob.cpp
<http://git.reviewboard.kde.org/r/104230/#comment9041>

    this is a no-go. rather, use the mimetype



plugins/grepview/grepjob.cpp
<http://git.reviewboard.kde.org/r/104230/#comment9044>

    put that declaration out of the conditional and reuse it in the else branch as well



plugins/grepview/grepjob.cpp
<http://git.reviewboard.kde.org/r/104230/#comment9042>

    doubled t



plugins/grepview/grepjob.cpp
<http://git.reviewboard.kde.org/r/104230/#comment9049>

    range instead of rng



plugins/grepview/grepjob.cpp
<http://git.reviewboard.kde.org/r/104230/#comment9046>

    use foreach(const QString& pattern, comments)



plugins/grepview/grepjob.cpp
<http://git.reviewboard.kde.org/r/104230/#comment9048>

    this is broken for lines that have comments somewhere else than at the start:
    
    foo, /* asdf, */ bar
    
    or
    
    asdf; // yxcv
    
    furthermore multiline comments like
    
    /*
    asdf
    */
    
    or
    
    <!--
    asdf
    -->
    
    are not detected



plugins/grepview/grepjob.cpp
<http://git.reviewboard.kde.org/r/104230/#comment9047>

    break early



plugins/grepview/grepjob.cpp
<http://git.reviewboard.kde.org/r/104230/#comment9045>

    please reuse the surrounding code style:
    
    if ()
    {
        ...
    }
    else
    {
        ...
    }
    
    note the {} in the else branch



plugins/grepview/tests/findreplacetest.cpp
<http://git.reviewboard.kde.org/r/104230/#comment9043>

    you will have to add unit tests for your new feature as well to make sure it works as intended


- Milian Wolff


On March 11, 2012, 8:59 p.m., Andrei G wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104230/
> -----------------------------------------------------------
> 
> (Updated March 11, 2012, 8:59 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> I've add the option for Find/Replace in Files to exclude comments from the results.
> This is my first patch so there could be some issues with it.
> 
> 
> Diffs
> -----
> 
>   plugins/grepview/grepdialog.h d4a1db6 
>   plugins/grepview/grepdialog.cpp b4b8cf5 
>   plugins/grepview/grepjob.h c357b77 
>   plugins/grepview/grepjob.cpp 66df1a9 
>   plugins/grepview/grepwidget.ui 94cfb00 
>   plugins/grepview/tests/findreplacetest.cpp ac0687d 
> 
> Diff: http://git.reviewboard.kde.org/r/104230/diff/
> 
> 
> Testing
> -------
> 
> I've tested on various files(text, .c, .php etc) with/without Exclude Comments option
> 
> 
> Screenshots
> -----------
> 
> Screenshot
>   http://git.reviewboard.kde.org/r/104230/s/460/
> 
> 
> Thanks,
> 
> Andrei G
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20120311/6304f5cd/attachment.html>