[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