[PATCH] Add support for JEDEC/metric standards to KLocale::formatByteSize

[Michael Pyne]

> Sorry Michael but after almost recovering from all those pesky computer
> problems preventing me to work on this issue, reading this was kind of
> disappointing.
> Basically, i think your patch is not overly elegant and implements this
> in a non-optimal way. There were some things in your patch done cleverer
> than in mine, and i really think we should have cooperated on merging
> both patches. There was no need to rush it in. 

Marcel,

I deeply apologize if that's the impression you came away with (and for the 
delay in my response -- I just returned from a weekend trip to my relatives).

I don't worry so much about what was done cleverer or not in a patch; indeed I 
try to keep my code as simple as possible and still be efficient and correct 
(although it's harder in Perl ;)

The patch itself has been under discussion for quite some time on kde-devel 
and kde-core-devel.  Eventually one has to decide to either ship it or not.

> You didn't even care to
> review my patch (http://reviewboard.kde.org/r/1003/) before committing
> yours.

Well the concepts were the same from what I could see, and even where we 
differed we weren't that much different after much thought.

For instance you included the union of all the binary unit types in your 
ByteSizeUnit enum, which is a valid choice with only 2 unit types even if it 
does lead to more case statements in your select.  When I opted for 3 unit 
types it didn't seem like a good idea to use the union of all 3 so I used just 
1 and used the unit type to distinguish which exact unit to use.

This meant I had to write code to handle auto-sizing right instead of letting 
gcc do the work though.  It also complicates the cache handling since not all 
options are present in the unit string cache (especially when switching to a 
different unit type).

The other part of your patch is something I did not commit (and therefore 
remains an outstanding feature request), the long-named options.  The 
implementation looks fine although I would argue that long-named units would 
be used so infrequently that there's no use caching them.  But that would make 
the implementation needlessly divergent so I see why you went that way.

However you are right in that I could have noted this directly on your review 
board entry so I apologize for not doing so there, especially while the 
mailing list discussion was still going on.

> I'm not going to get overly emotional about this, but as someone
> just starting to get in touch with kde-core programming, and having
> spent (now: wasted) quite some time thinking this through i feel
> overran,

You and me both...

I wish to make this abundantly clear: the fact that you have actual code 
patches, to kdelibs (kdecore no less!) puts you a thousand times ahead from my 
perspective.  Many people like to complain about this or that on mailing lists 
but can't even be bothered to even setup a development environment to test 
other patches, let alone work on it.  But you've done that, and that's the 
kind of thing that gets noted, and something we need more of in KDE. :)

But at the same time what I've committed does not make your patch completely 
obsolete, as for instance there is the long unit names your patch had.  I 
didn't leave those out of my patch because I don't think they should go in, 
but because that wasn't necessary for what I was trying to achieve.  Had your 
work been committed in February then my patch would still have been required 
(although it would have been simpler!).

I would like to see your name in the kdelibs copyright headers, even if it 
doesn't come as a result of this patch in particular.  So I really hope you 
don't take away only negative feelings from this thread. :-/  I don't know if 
this patch is your only goal in coding or if you were going to be programming 
anyways but your time is never at the end wasted if you still got something 
out of it.  I've had patches that had to be rejected, changed substantially, 
or even ripped out after the fact and although it's always unpleasant I can't 
say it was wasted time for me, as I learned something every time.

> [i feel overran] and not by the technically best implementation aswell.

As always I would appreciate technical feedback -- it's not too late for you 
to fix my shortcomings. ;)

> Introducing new public APIs shouldn't be done this hastily.

"hastily" is hardly the way to describe this particular series of KLocale 
threads...

> Furthermore,
> i might have just aswell committed my own patch 'in time' (before you
> committed yours) but didn't, in favor of merging both patches and not to
> disrespect your work.

Until the tarballs are released nothing is final.  So in that regard as long 
as there is general agreement it's better to get the code in earlier than 
later as it increases the changes that breakages will be found.  Over the 
course of the 4.4 timeframe if we find that you have found technical issues 
then the patch may even end up substantially completely your style (i.e. union 
set of the unit types).  But I'm also not going to try to merge two patches at 
the same time as that way lies madness.

I assure you I meant no disrespect to you or your work.  After all, I have not 
added myself even to the copyright headers in this case -- I'm not looking for 
credit, just to get the code added to resolve the issues that have been 
presented by quite a few users in a way that is as equitable as possible to as 
many as possible.  And as I've mentioned, nothing is final until tarballs hit 
the disks so if we find some issue incompatible with kdelibs it can always be 
ripped right back out.  So, I'm under no illusions that merging the code 
magically protects it from expulsion.  (Needless to say I don't think that 
would happen in this case otherwise I wouldn't have merged it.  But still.)

Regards,
 - Michael Pyne
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20090720/47566831/attachment.sig>