Review Request 118481: Basic integration of Bazaar VCS

[Kevin Funk]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118481/#review59015
-----------------------------------------------------------


Code looks globally decent. Nice work!

Some general remarks:
* Please prefix names for class members with 'm_'
* Use Q_UNUSED where possible

And: Please add tests for this. This is quite easy to do, just have a look at 'gittest.cpp'. I know, it's tedious, but especially for this kind of functionality it's a must-have (bzr output may change, and we wouldn't notice, etc.)


plugins/bazaar/src/bazaarplugin.cpp
<https://git.reviewboard.kde.org/r/118481/#comment41053>

    License is wrong, isn't it? The copyright says it's GPL'd



plugins/bazaar/src/bazaarplugin.cpp
<https://git.reviewboard.kde.org/r/118481/#comment41052>

    Q_UNUSED



plugins/bazaar/src/bzrannotatejob.h
<https://git.reviewboard.kde.org/r/118481/#comment41055>

    I think it's fine to use a int (for currentLine) + QHash (for commits) here instead. Avoids the ugly static_cast later and instead makes consistent use of the Qt containers.



plugins/bazaar/src/copyjob.cpp
<https://git.reviewboard.kde.org/r/118481/#comment41047>

    Q_UNUSED (more of these issues in this patch)



plugins/bazaar/src/copyjob.cpp
<https://git.reviewboard.kde.org/r/118481/#comment41048>

    Not needed.



plugins/bazaar/src/kdevbazaar.desktop.cmake
<https://git.reviewboard.kde.org/r/118481/#comment41049>

    That's some easy-to-remember mail btw ;)



plugins/bazaar/src/kdevbazaar.desktop.cmake
<https://git.reviewboard.kde.org/r/118481/#comment41056>

    GPL?



plugins/bazaar/src/utilities.h
<https://git.reviewboard.kde.org/r/118481/#comment41050>

    Put those functions in a namespace (e.g. "BazaarUtils", rename file accordingly.
    
    Documenting them that would be helpful as well. It's not easy to understand what their purpose is without context.



plugins/bazaar/src/utilities.cpp
<https://git.reviewboard.kde.org/r/118481/#comment41051>

    Minor: const& QString


- Kevin Funk


On June 2, 2014, 6:51 p.m., Maciej Poleski wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118481/
> -----------------------------------------------------------
> 
> (Updated June 2, 2014, 6:51 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> This patch adds basic integration of Bazaar VCS.
> 
> It provides implementation of KDevelop::IDistributedVersionControl interface as a plugin.
> It works by forwarding (user) requests to bzr executable (which must be in PATH) and parsing (text output) result from spawned bzr process.
> 
> 
> Diffs
> -----
> 
>   plugins/CMakeLists.txt e0ccbc1 
>   plugins/bazaar/CMakeLists.txt PRE-CREATION 
>   plugins/bazaar/bazaar.kdev4 PRE-CREATION 
>   plugins/bazaar/src/CMakeLists.txt PRE-CREATION 
>   plugins/bazaar/src/bazaarplugin.h PRE-CREATION 
>   plugins/bazaar/src/bazaarplugin.cpp PRE-CREATION 
>   plugins/bazaar/src/bzrannotatejob.h PRE-CREATION 
>   plugins/bazaar/src/bzrannotatejob.cpp PRE-CREATION 
>   plugins/bazaar/src/copyjob.h PRE-CREATION 
>   plugins/bazaar/src/copyjob.cpp PRE-CREATION 
>   plugins/bazaar/src/diffjob.h PRE-CREATION 
>   plugins/bazaar/src/diffjob.cpp PRE-CREATION 
>   plugins/bazaar/src/kdevbazaar.desktop.cmake PRE-CREATION 
>   plugins/bazaar/src/utilities.h PRE-CREATION 
>   plugins/bazaar/src/utilities.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/118481/diff/
> 
> 
> Testing
> -------
> 
> Only manual testing
> 
> 
> Thanks,
> 
> Maciej Poleski
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20140602/1df221a2/attachment.html>