[PATCH] Add support for JEDEC/metric standards to KLocale::formatByteSize
mpyne at kde.org
Tue Jul 21 02:57:26 BST 2009
> Sorry Michael but after almost recovering from all those pesky computer
> problems preventing me to work on this issue, reading this was kind of
> 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.
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
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
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
> 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.)
- Michael Pyne
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 836 bytes
Desc: This is a digitally signed message part.
More information about the kde-core-devel