D23579: WIP: port ftp slave to new error reporting system

[Harald Sitter]

sitter created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
sitter requested review of this revision.

REVISION SUMMARY
  This system is a bit inspired by an earlier patch I threw together in
  a half hour to prevent the actual logic code from calling state-changing
  functions by splitting the interface class (implements SlaveBase) from
  the actual FTP client implementation (used by the interface class).
  The advantage here is that the future internal code cannot call error
  or finished directly (and cause state violations in SlaveBase - e.g.
  double-error call) because there is no trivial way to access them.
  
  Instead the internals return a new Result class if they need to return
  anything. This is largely inspired by how Error handling works in Go.
  The caller of an internal function needs to check the result or explicitly
  disregard it. The caller may then decide to ignore an internal error result
  (e.g. chmod after upload failed), create its own higher level result, or
  forward it unchanged. On the interface level we then convert the result
  to appropriate state changes of the slave.
  This is assisted by Q_REQUIRED_RESULT so capable compilers will warn if
  a Result is not explicitly ignored as that'd be an indication of missing
  error handling in new code, again future proofing the slave.
  
  All things put together this turned out really well I think.
  
  This also adds a primitive test because it broke a million times while
  porting. Starts a simple ruby ftpd (with some monkey patching applied).
  
  TODO:
  
  - there are still a bunch of pending todos littered over the code
  - currently the internal class relies on helper functions to forward interface calls to the interface class, they can all be dropped in favor of directly calling `q->foo`.
  - needs more test scenarios I'd imagine
  - there is a larger question if we still need the StatusCode enum, it was exclusively used to differentiate when the error is remote vs local so the correct file can be referenced (e.g. "/foo failed to read" vs "ftp://h/foo failed to read") but since we can now create construct the correct error strings even in very deep internal call chains this may no longer be necessary
  - I am wondering if maybe returning QScopedPointer<Result> would be nicer, currently there are a lot of useless copies around
  - Result::fail needs being made more explicit, all three arguments should always need providing and ideally be useful (i.e. QString() the errorString is fairly ugh
  - unrelated: fix unrelated warnings, there's a lot of unsigned<->signed coercion happening

REPOSITORY
  R241 KIO

BRANCH
  ftp

REVISION DETAIL
  https://phabricator.kde.org/D23579

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/ftp/testCopy1
  autotests/ftp/testCopy2
  autotests/ftp/testOverwriteCopy1
  autotests/ftp/testOverwriteCopy2
  autotests/ftpd
  autotests/ftptest.cpp
  cmake/FindGem.cmake
  cmake/FindGem.cmake.in
  cmake/FindRubyExe.cmake
  src/ioslaves/ftp/ftp.cpp
  src/ioslaves/ftp/ftp.h

To: sitter
Cc: dfaure, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190830/6927a952/attachment-0001.html>