----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129170/#review99986 -----------------------------------------------------------
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 read. 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) <https://git.reviewboard.kde.org/r/129170/#comment67144> Let's make this one less technical. Poor translators are going to wonder what kHeader means. src/k7zip.cpp (line 2588) <https://git.reviewboard.kde.org/r/129170/#comment67145> Is this explicit QString{ necessary? src/karchive.h (line 89) <https://git.reviewboard.kde.org/r/129170/#comment67146> @since 5.28 src/karchive.h (line 283) <https://git.reviewboard.kde.org/r/129170/#comment67147> @since 5.28 src/karchive.h (line 292) <https://git.reviewboard.kde.org/r/129170/#comment67148> remove src/karchive.cpp (line 125) <https://git.reviewboard.kde.org/r/129170/#comment67149> 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) <https://git.reviewboard.kde.org/r/129170/#comment67150> 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 > >
