Differential D9672

Don't emit error signal before we tried all list commands.
ClosedPublic
Actions

Authored by wcpan on Jan 5 2018, 3:49 AM.

Details

Reviewers
dfaure
Summary

Use a special error code to skip error reporting.

BUG: 387634

Diff Detail

Repository
R241 KIO
Branch
fix-ftp-list
Lint
No Linters Available
Unit
No Unit Test Coverage
wcpan created this revision.Jan 5 2018, 3:49 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 5 2018, 3:49 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
wcpan requested review of this revision.Jan 5 2018, 3:49 AM
cfeck added a subscriber: cfeck.Jan 31 2018, 12:51 AM

Sorry for the late response, but we currently have no maintainer for the FTP code in KIO.

From quickly checking, the intention looks good, but if the ERR_* codes are part of an enum, maybe add the special value -1 with a name there, instead of hardcoding the -1 value? It might even fail with compilers that strictly treat enum's as a separate data type.

cfeck edited the summary of this revision. (Show Details)Jan 31 2018, 12:52 AM
dfaure requested changes to this revision.Feb 3 2018, 12:27 AM

ERR_* is KIO-wide, and isn't technically an enum.

But the proper error code for no error is KJob::NoError (which is 0). Can you switch to that, and adjust the second comment, and kill the first comment altogether? (redundant with code and with 2nd comment)

Thanks! (and sorry for the delay, too)

This revision now requires changes to proceed.Feb 3 2018, 12:27 AM
wcpan updated this revision to Diff 26460.Feb 3 2018, 7:33 PM

Don't emit error signal before we tried all list commands.

Use a special error code to skip error reporting.

dfaure accepted this revision.Feb 4 2018, 5:04 PM
This revision is now accepted and ready to land.Feb 4 2018, 5:04 PM
aacid closed this revision.May 20 2018, 10:03 PM
aacid added a subscriber: aacid.

This already landed in bc4ea213b1d948b921236db29e78846c34496db7

Restricted Application edited subscribers, added: kde-frameworks-devel; removed: Frameworks. · View Herald TranscriptMay 20 2018, 10:03 PM

Revision Contents
Changeset List

PathPackages
Msrc/ioslaves/ftp/ftp.cpp (8 lines)

Diff 26460

View Options

src/ioslaves/ftp/ftp.cpp

Loading...