On Thu, 17 Apr 2014, Dionysios Kalofonos wrote:

> The attached patch fixes this issue, where the library would return
> only the first line of a response to a custom request that included a
> string literal (http://tools.ietf.org/html/rfc3501#section-4.3).
>
>
> Could i have your feedback before i continue with unit tests etc?

Sure.... and thank you for you patch.

I'll split my feedback/thoughts into two sub-sections:

A) Implementation

I'm not convinced having a set of custom functions is the best approach. I contemplated this when Jiri started extending Daniel's initial IMAP implementation and I steered away from it then.

Now I appreciate that we don't have it correct at the moment by imitating a LIST command, and I appreciate you are trying to fix my decision there ;-) What you have implemented is certainly a lot better than that but if we view a custom request as the ability to override any command then we should also allow APPEND (the upload) to be overridden by a custom request as well.

I wouldn't like to proceed down the "custom" state / perform / resp function route only to rewrite it for a third time in the future without airing some other possibilities ;-)

As such.... Is there a way that we can specify the custom request but call the existing functions?

For example:

* Modify the decision process in imap_perform() to ignore the custom variable.
* In imap_perform_fetch(), imap_perform_list(), imap_perform_append() we send the contents of custom if the custom variable is set and send the existing command if it isn’t?
* I appreciate the responses may be a little different and that might make the imap_endofresp() function a little complex :(

B) Coding Style

Generally speaking your coding style doesn't match that of curls:

* Curly brackets for code segments such as if statements are not in the correct place for curl. We follow the old school way of doing it for those blocks and have the start brace on the end of the if/for/while statement.
* Whilst we have a mixture of code that quickly exits a function after a test condition and code that performs if/else I have tried to maintain a consistent style in the email protocols. I would recommend the if in imap_endofresp() be an if/else and let the return TRUE at the end of the function execute in it's natural code path.
* We also have a mix of placing the constant of an if condition on the left and on the right - again I have tried to be consistent in the email protocols using RHS rather than LHS.
* You have used a mixture of tabs and spaces for indentation - please use spaces.
* The indentation of imap_state_custom_resp() seems a little strange - quite possible if you have tabs set as 8 characters in your editor?

Other than that it's a pretty good first stab ;-)

Cheers again and I forward to hearing your thoughts on my points in A).

Kind Regards

Steve

Steve

-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2014-04-17