sitter added inline comments.
INLINE COMMENTS
> dfaure wrote in ftptest.cpp:186
> what's bad quot?
>
> Is this test about a bug, or the job failing is what we want?
That was meant to read quota. I've changed it to QVERIFY(!job->exec()); as per
your earlier comment.
> dfaure wrote in ftp.cpp:696
> The reuse of the `result` var irks me a bit.
> const vars rock :-)
>
> Would it be a bad idea to make Result implicitly convertible to bool, so you
> can keep doing this inside the if? ` if
> (!ftpSendCmd(QByteArrayLiteral("PWD")) || (m_iRespType != 2)) {` would still
> work fine then.
>
> This would simplify all cases where we test for failures, but ignore the
> actual error message on failure (because we'll provide a more high-level one,
> like here). In other cases, we indeed need a result var.
I know what you mean, I originally started out with consts but moved away
because it was getting fairly repetitive and in the end the constness offers
little, the objects are fairly "ephemeral" much like errno.
The result handling is indeed very "wordy", but I think there is value to be
had in the expressiveness there:
A Result is not really a binary affair. Yes it is failure or success, but if it
is a failure the developer **must** make a conscious choice to not use the
context information of. If Result was implicitly usable as a bool we'd not
force this conscious choice, opening us up for mistakes further down the line
when everyone has forgotten if ftpSendCmd needs/deserves result handling... or
worse yet, looking at old code it won't be obvious if the author ignored the
result context by accident or intentionally.
So, I would stay away from treating a Result like a bool.
To get to the heart of the issue though: I actually think ftpSendCmd probably
shouldn't even return a Result but rather a bool. Probably 99% of all callers
entirely ignore the Result context and treat it as a bool, which seems like a
strong indication that the return type is unnecessarily complex. Should I go
ahead with making it bool?
> dfaure wrote in ftp.cpp:1602
> What happened to those two lines of comments? Can I suggest to keep them?
Oh! Collateral damage from removing the nesting I guess. Comments are back now.
> dfaure wrote in ftp.cpp:1880
> Wasn't that just a way to propagate the error from the low-level method to
> this method? Your new mechanism replaces that, no?
I could have sworn there was code that differentiated between server and
client. I can't find it anymore... maybe it was all a dream, or maybe I saw it
in the history.
Oh well, down with the useless enum!
> dfaure wrote in ftp.cpp:2363
> optimistic but useless initializations, all code paths set (or ignore) result.
I do need to declare it though, and by design a Result cannot have a "null"
state, so I either need to initialize a failure or pass here. Do you have a
better suggestion?
> dfaure wrote in ftp.cpp:2596
> Yes, clearly. But the question is where to check it.
>
> Hmm, how does this method even get called, without an event loop? From
> `m_control->waitForReadyRead`, maybe?
>
> The old code was broken here, it would for sure emit error+error or
> error+finished, since the main code had no idea that this slot emitted error
> already.
>
> Hmm. Shouldn't we do this connect *before* the waitForConnected in
> Ftp::synchronousConnectToHost? If a proxy exists, it will surely act at
> connection time?
> And then it becomes easier, the place where to check the errno-like member is
> just after the call to Ftp::synchronousConnectToHost...
Since you mention it... where even is the eventloop for any of this?
SlaveBase has no event processing, neither does the FTP slave. I get the
distinct feeling that the entire auth code never worked, or at least not since
kf5. To that end I'd actually be in favor of removing it since no one
complained (or I cannot find any bug report anyway).
From some limited testing your assessment would be mostly correct though. When
trying to waitForConnected there'd be a ProxyAuthenticationRequiredError. Which
we can handle somewhat awkwardly by querying the info from the user and then
setting a newly "enhanced" proxy QNetworkProxy::setApplicationProxy before
creating a new socket I guess.
I'm not too excited about fixing proxy auth support no one apparently needs.
Originally introduced without review as well:
https://git.reviewboard.kde.org/r/102923/
> dfaure wrote in ftp.cpp:2706
> This looks wrong. openConnection() should emit connected() or error(), but
> never finished().
You may wish to take a look at D23537 <https://phabricator.kde.org/D23537> ;)
> dfaure wrote in ftp.h:70
> I think you can make all members const?
>
> (This will prevent bypassing the "factory methods"...)
It's a good idea. Would that work though?
Currently the results rely on the implicit move operator when collecting the
returned result
result = ftpGet()
if the members are const we couldn't move/copy like that anymore.
If we consider the mutability a problem I think I'd just make the members
private and give them getters. I don't mind much either way.
> dfaure wrote in ftp.h:156
> Does this method serve any purpose?
Its sole purpose is to make it harder to call
`q->finished()` in the internal class (and that way bypass the result system).
Same for error() above.
> anthonyfieroni wrote in ftp.h:167
> Should be hidden?
Does that make a useful difference for a plugin?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D23579
To: sitter, dfaure
Cc: anthonyfieroni, dfaure, kde-frameworks-devel, LeGast00n, GB_2, michaelh,
ngraham, bruns
