[Kde-pim] Review Request: Add a safe ensureParsed() method to KMime::Contents

[Szymon Stefanek]

> On Feb. 25, 2012, 8:51 p.m., Ingo Klöcker wrote:
> >
> 
> Ingo Klöcker wrote:
>     ReviewBoard or my old Konqueror has eaten the following comments:
>     
>     Did you write unit tests for this? Is everything else already checked with unit tests? Do they pass? If any of those questions is answered with no, then I'm not confortable with your change. In particular, because you seem to have missed a few spots.
> 
> Szymon Stefanek wrote:
>     Hum.. I didn't write unit tests nor I have run the existing ones.
>     I'm just trying to fix a bug that makes me nervous :D
>     
>     This change doesn't modify the behavior of any existing function though. In fact
>     it was explicitly written this way to avoid touching any existing code path.
>     
>     I might write an unit test for the new function later, when I find some
>     time to figure out how it has to be done...
>     
>     Having said this, I'm certainly willing to fix the issues you've found
>     and thank you for looking at this patch :)
>
> 
> Ingo Klöcker wrote:
>     Granted, your changes do not modify the behavior of existing functions. This does not save you from writing a unit tests for the new function. Do it now or wait for somebody else to approve your patch. If you don't write the test now you'll never going to write it. The point of the unit test is not to prove that your function works now. (For this you should have written the unit test before you implemented the function.) It's to make sure that it does still work as intended 10 years from now.

I understand your point about tests and you're right.

However I've looked briefly into this and it seems that
to make meaningful tests I'd need to add at least another
function to the library. Then I'd need to post another review
request, wait some more days, maybe fix it because someone
doesn't like the syntax... Then write the tests for all
the possible combination of calls in order to... well...

Unfortunately in the next days I'll not have the time to do
all of this and quite frankly I've already wasted several
hours of my life over this bug.

Scratch this patch. I've found a very quick workaround that
fixes #291171 for me and doesn't touch the library.

( see https://git.reviewboard.kde.org/r/104090/ ).


- Szymon


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104078/#review10901
-----------------------------------------------------------


On Feb. 25, 2012, 10:57 p.m., Szymon Stefanek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104078/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2012, 10:57 p.m.)
> 
> 
> Review request for KDEPIM and KDEPIM-Libraries.
> 
> 
> Description
> -------
> 
> This patch adds an ensureParsed() method to KMime::Contents.
> Unlike parse(), ensureParsed() can be called multiple times without
> breaking the message body.
> 
> So in turn users of KMime::Contents that do not know if parse() has been
> called on the message they are handling can always call ensureParsed().
> 
> This is a part of a larger fix for bug 291171
> 
> 
> This addresses bug 291171.
>     http://bugs.kde.org/show_bug.cgi?id=291171
> 
> 
> Diffs
> -----
> 
>   kmime/kmime_content.h 05f67c2 
>   kmime/kmime_content.cpp 37ca474 
>   kmime/kmime_content_p.h f09d293 
> 
> Diff: http://git.reviewboard.kde.org/r/104078/diff/
> 
> 
> Testing
> -------
> 
> Compiled and tested. Along with another patch to kdepim it fixes bug 291171.
> 
> 
> Thanks,
> 
> Szymon Stefanek
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/