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 _______________________________________________ tails-dev mailing list tails-dev@boum.org https://mailman.boum.org/listinfo/tails-dev