[PATCH 6/6] Convert everything to use Size.
David Lehman
dlehman at redhat.com
Thu Jan 9 20:00:53 UTC 2014
On Wed, 2014-01-08 at 12:45 -0500, Anne Mulhern wrote:
> Comments are:
>
> 1) I'ld update the DeviceFactory comments to be precise about the type of the size parameter of the constructor. It's Size, which seems fine to me.
Fair enough. I should have updated that as part of the sphinx patch set.
Done locally.
> 2) There's a few hunks failing wrt master that I can't really comment on.
I haven't rebased in a while. I doubt it's anything serious.
> 3) In class StorageDevice I think that _targetSize may have varying types, i.e., not all Size. The type of newsize in _setTargetSize could easily not be size. Maybe _targetSize could be set by setter which could make sure value set is a Size.
I added a check to all the _setSize and _setTargetSize methods to make
sure the new size is a Size instance and raise ValueError if not.
> 4) I'm not sure about where newLen is passed to parted.Geometry constructor in _computeResize. It probably should not be a Size there, but it very likely is.
Right. Fixed with a call to int().
> 5) Same caveat about method _setSize; not sure about type of newsize parameter or how values calculated from that type should be passed to parted functions.
Likewise.
> 6) Ext3FS._maxSize should be GiB, not TiB or else Ext2FS._maxSize should be TiB not GiB. There's a few more _maxSize's where the changes from numeric values to Sizes seems inconsistent.
Ext2FS should have been TiB. I went back through and didn't see any
others that looked wrong.
> 7) I was a bit confused by the instances of "kib" prefix in udev.py. Its not KiB and it's not kibi either.
The parsing is case-insensitive, but I fixed it for better readability.
> 8) Tiny, but that closing */ on line 354 of util.py in the comment "every machine has the total RAM..." clearly belongs to another language.
Meh.
>
> More generally:
>
> 1) I'm looking forward to this patch saving a ton of trouble in the long run.
> 2) I bet it could all be done w/ int instead of Decimal but that's a challenge for another day.
>
> - mulhern
>
>
>
>
>
>
>
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
More information about the anaconda-patches
mailing list