Hi,
intrigeri wrote (22 Nov 2013 21:21:44 GMT) :
> Andres Gomez Ramirez wrote (21 Nov 2013 16:03:58 GMT) :
>> Ok no problem, attached is the patch for "Persistence: display nicer paths"
>> feature -> https://labs.riseup.net/code/issues/5311.
> Cool, thanks. I'm very happy to see someone else than me starting to
> touch our Perl code in non-trivial ways :)
> Here's an initial review.
> First, it seems to me that this patch is based on an older,
> pre-split-to-tails-perl5lib, version of tails-persistence-setup.
> First thing is then to rebase it on top of the current master branch.
> Sorry for the bad timing :/
>> +has 'partition_file' =>
>> + lazy_build rw Str,
>> + documentation => q{The persistent partition file, e.g. /dev/sdb1.};
> This attribute's name seems unclear to me. First, it should be
> persistence_partition_* for consistency with other similar attributes.
> For the rest of the name, I have no particularly awesome proposal,
> perhaps simply persistence_partition_device_file, to match
> udisks' wording?
> Also, this attribute should have the NoGetopt metaclass, since it is
> not suitable to be set on the command-line.
>> +sub _build_partition_file {
>> + my $self = shift;
>> +
>> + my $device_file = $self->get_device_property($self->device, 'DeviceFile');
>> + my $partition_number =
>> $self->get_device_property($self->persistence_partition,
>> 'PartitionNumber');
>> +
>> + $device_file.$partition_number;
> This naming scheme is not correct for all kinds of supported devices,
> e.g. SD cards plugged into a reader wired via SDIO. See commit
> 83637f4e for details. Doesn't $self->persistence_partition have
> a DeviceFile property that we could use instead of manually appending
> the partition number this way? If it has, let's simply use it. Else,
> using the info from commit 83637f4e should be good enough.
> Also, DeviceFilePresentation might be nicer in some cases that we
> might want to support in the future, so I would pick that one
> personally instead of DeviceFile, if it is supported by
> Squeeze's udisks.
>> has 'persistence_partition_size' => required ro Int;
>> -
>> +has 'partition_file' => required ro Str;
> The two empty lines between attributes and constructors was there on
> purpose. I'm not pretending the whole codebase is consistent, but I'm
> trying to have two empty lines before each =head1. Refraining from
> doing unrelated whitespace changes in a patch is generally a good
> thing, anyway.
> I can't wait to see the second iteration! :)
Ping?
(No big emergency, as this is material for 0.23, that should freeze
in February.)
Cheers,
--
intrigeri
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc
