This is an automatically generated e-mail. To reply, visit:

Fix it, then Ship it!

I like this very much. qDebug or qWarning is not the right thing to do for 
something that is not an error in the application code but in the data being 
Well, some of these are about errors in application code (calling close() twice 
etc.), so these *could* be qWarnings in addition (especially in case the 
application code also doesn't check errorString() properly).
But the errorString() mechanism is definitely good for all data-related errors, 
I like it (and then it makes sense to always set an error string when returning 
false, also in cases of a programmer error).
I was considering suggesting an enum in addition, but I'm not sure it's really 
useful. I can't think of a case where the application will do something 
different based on the enum value.

src/k7zip.cpp (line 2409)

    Let's make this one less technical. Poor translators are going to wonder 
what kHeader means.

src/k7zip.cpp (line 2588)

    Is this explicit QString{ necessary?

src/karchive.h (line 89)

    @since 5.28

src/karchive.h (line 283)

    @since 5.28

src/karchive.h (line 292)


src/karchive.cpp (line 125)

    Why not use QStringLiteral here, rather than QString{QLatin1String{}}? It's 
faster at runtime (the conversion to 16-bit characters happened at compile 
time). But well, you're going to use tr() anyway ;)

src/karchive.cpp (line 155)

    Yes, as long as the method returns false, it's an error ;) This comment is 
funny but redundant.

- David Faure

On Oct. 13, 2016, 4:45 a.m., Romário Rios wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129170/
> -----------------------------------------------------------
> (Updated Oct. 13, 2016, 4:45 a.m.)
> Review request for KDE Frameworks.
> Repository: karchive
> Description
> -------
> This method is similar to `QIODevice::errorString()`. I added a public 
> `errorString()` method and a protected `setErrorString()` method, to allow 
> `KArchive`'s subclasses to implement their own error messages. I also 
> implemented most error messages from most subclasses.
> Diffs
> -----
>   autotests/karchivetest.cpp d0fbf41 
>   src/k7zip.cpp 692b1db 
>   src/kar.cpp 7204fb1 
>   src/karchive.h b528a4a 
>   src/karchive.cpp a1a160a 
>   src/karchive_p.h 256620d 
>   src/krcc.cpp 1947dd6 
>   src/ktar.cpp f70b155 
>   src/kzip.cpp 94d4276 
> Diff: https://git.reviewboard.kde.org/r/129170/diff/
> Testing
> -------
> I added `QVERIFY` calls after all errors in `karchivetests.cpp`. Perhaps 
> we'll need more tests, but I'm not sure how to make an archive to fail in 
> some specific way aside from the very basics ("file not found", etc.).
> Thanks,
> Romário Rios

Reply via email to