Review Request: Review API of the new Path class

[Milian Wolff]

> On Jan. 5, 2013, 11:11 a.m., Andreas Pakulat wrote:
> >

Many thanks for that - I appreciate such reviews!


> On Jan. 5, 2013, 11:11 a.m., Andreas Pakulat wrote:
> > project/path.h, line 92
> > <http://git.reviewboard.kde.org/r/108196/diff/1/?file=104951#file104951line92>
> >
> >     Is the string-conversion really needed? I would've hoped that most of the time we're dealing with KUrl's already.

Not if you operate on local files, e.g. Qt uses QString for local paths everywhere. Same for QFileInfo or IndexedString.


> On Jan. 5, 2013, 11:11 a.m., Andreas Pakulat wrote:
> > project/path.h, line 117
> > <http://git.reviewboard.kde.org/r/108196/diff/1/?file=104951#file104951line117>
> >
> >     What happens here when the subPath is a full url or a sub-path with multiple segments?

Sub path is expected to be a path, not a full url. The string will just be appended. Furthermore it does currently not support relative nor absolute subPaths. I'll add documentation and unit tests for that.

Do you think these cases should be supported?


> On Jan. 5, 2013, 11:11 a.m., Andreas Pakulat wrote:
> > project/path.h, line 193
> > <http://git.reviewboard.kde.org/r/108196/diff/1/?file=104951#file104951line193>
> >
> >     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).

For local files this is much faster than a KUrl. And that is neat for checks in e.g. QFile::exists or for setting the text in a label or such.

You sould take a look at the benchmarks, KUrl is just dog slow.


> On Jan. 5, 2013, 11:11 a.m., Andreas Pakulat wrote:
> > project/path.h, line 279
> > <http://git.reviewboard.kde.org/r/108196/diff/1/?file=104951#file104951line279>
> >
> >     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.

I agree, I did this to follow the KUrl API as closely as possible. I'll add these methods and deprecate fileName.


> On Jan. 5, 2013, 11:11 a.m., Andreas Pakulat wrote:
> > project/path.h, line 296
> > <http://git.reviewboard.kde.org/r/108196/diff/1/?file=104951#file104951line296>
> >
> >     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.

Again, I followed the KUrl API to reduce the porting effort. I also prefer parent, but in KDevelop the "const" is quite easily seen when calling the function (hover it, or in code completion, ...).

I'll add the parent and deprecate up.


> On Jan. 5, 2013, 11:11 a.m., Andreas Pakulat wrote:
> > project/path.h, line 311
> > <http://git.reviewboard.kde.org/r/108196/diff/1/?file=104951#file104951line311>
> >
> >     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.

I also thought about that but deliberately decided against it for space reasons (saving one pointer for local paths). But I agree that it might be a bit over-optimized. I'll revisit the code and take another look at it.


- Milian


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


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/0769cde9/attachment.html>