Hi,
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! :)
Cheers,
--
intrigeri
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc
