Review Request: Review API of the new Path class

[Andreas Pakulat]

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



project/path.h
<http://git.reviewboard.kde.org/r/108196/#comment18995>

    Is the string-conversion really needed? I would've hoped that most of the time we're dealing with KUrl's already.



project/path.h
<http://git.reviewboard.kde.org/r/108196/#comment18996>

    What happens here when the subPath is a full url or a sub-path with multiple segments?



project/path.h
<http://git.reviewboard.kde.org/r/108196/#comment18997>

    Is this really needed? I don't like transforming a Path to a String due to loosing all type information. I think we should only allow accessing individual path segments (like a list) or getting a fully-qualified KUrl out of the clas (and into it).



project/path.h
<http://git.reviewboard.kde.org/r/108196/#comment18998>

    Same thing as pathOrUrl.



project/path.h
<http://git.reviewboard.kde.org/r/108196/#comment18999>

    Same thing as pathOrUrl.



project/path.h
<http://git.reviewboard.kde.org/r/108196/#comment19000>

    I'm always confused when seeing filename/setFilename and dealing with things where there can also be a directory in the last segment of the path. I think the functions should rather be called lastSegment/setLastSegment or be removed completely and allow setting of any segment using an index-based API.
    
    It just feels weird to do a setFileName to change the last directory in a path.



project/path.h
<http://git.reviewboard.kde.org/r/108196/#comment19001>

    I always wonder wether up just returns a new value or also changes the state of the object in-place. I'd prefer a parent() getter which makes it clearer that the object is not changed. Yes I know the const indicates there's no in-place change but you don't see that when calling the function.



project/path.h
<http://git.reviewboard.kde.org/r/108196/#comment19002>

    Hmm, I think I'd separate the remote-url part from the path itself here. There are some places in the code where you have explicit checks for remote-urls and then take the first or second item from the list. Having a separate member would make that code a lot clearer.


- Andreas Pakulat


On Jan. 5, 2013, 10:59 a.m., Andreas Pakulat wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108196/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2013, 10:59 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> Discuss API of the new Path class
> 
> 
> Diffs
> -----
> 
>   project/path.h PRE-CREATION 
>   project/path.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/108196/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andreas Pakulat
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20130105/b3b88139/attachment-0001.html>