I don't understand why people are reluctant to use techniques like version codes here. (I say this because ive noticed others also dislike version codes.) To me a settings field is just a data format which is really a file format. And its usually better to code version strings into such systems early while its easy. The flexibility such a mechanism offers in a situation like here is quite nice if you ever have reason to change the way things work. And I've had call to use it elsewhere on PM to good effect already.

# note that these two vars go together to a certain extent.
my $magic="===&01";
my $escape="%=&";

sub unpack_hash {
  my ($str)=shift;
  my %hash;

  if (substr($str,0,6) eq $magic) {
    substr($str,0,6)='';
    %hash=map { map { 
                      s/%(\d)/substr($escape,$1,1)/ge;
                      $_
                   } split /=/,$_ 
                 } split /&/,$str;
    
  } else {
    # old getVars() support...
  }
}

sub pack_hash {
  my $hash=shift;
  return $magic.join("&",map { 
    join "=",map { 
         (my $v=$_)=~s/([$escape])/'%'.index($escape,$1)/ge 
         $v
    } $_ => $hash{$_}     
  } sort keys %$hash);
}
[download]

Since bullet proofing getVars() probably means it faster to do than the current method, I personally dont see why we shouldn't do it.

My reluctance is based on two things, both very minor; first, it means taking really short simple code and making it less so; second, it reduces the readability of the raw data.

It's your call; if you say so, I'll take what you have above and integrate it with my unpackVars/packVars (though I'd much prefer just to use the escape() and unescape() that are already there).

it means taking really short simple code and making it less so;

Except that less code is actually executed and there is less stack overhead.

second, it reduces the readability of the raw data.

How does it do that? The current mechanism uses url encoding to handle \W chars. This mechanism would leave everything alone except for =,& and %. (Pick three less common chars if you want... I did in the personal nodelet stuff.)

Im not saying you should definatively do anything, im just arguing that my suggestion is sound. Im hoping others pipe up with their thoughts too (although really only people that use setting's will have any experience to guide their thoughts). My position is that making my proposed changes would make a lot of things more robust and easy to deal with and actually be a marginaly net gain in efficiency due to less code and faster encoding. I dont see why we would give up those benefits for what we have now given the relative costs.



---
demerphq


First they ignore you, then they laugh at you, then they fight you, then you win.
-- Gandhi

Flux8

How does this look (patch is on top of [id://patchable settings;displaycode|previous patch]):

--- Everything.pm    2004-10-05 11:28:38.716515200 -0700
+++ Everything.pm.new    2004-10-05 11:31:58.093204800 -0700
@@ -261,13 +261,28 @@ sub unescape
 sub unpackVars
 {
     my ($vars) = @_;
+    my $format_version = "00";
+
+    # version 00: original format
+    # version 01: keys are escaped, not just values
+    $format_version = $1 if $vars =~ s/^==(\d\d)&//;
 
     return {} unless $vars;
 
-    my %vars = map { split /=/ } split (/&/, $vars);
-    foreach (keys %vars) {
-        unescape $vars{$_};
-        if ($vars{$_} eq ' ') { $vars{$_} = ""; }
+    my %vars;
+
+    for (split /&/, $vars) {
+        my ($k,$v) = split /=/;
+
+        if ($format_version > 0) {
+            unescape( $k, $v );
+        } else {
+            unescape( $v );
+        }
+
+        $v = '' if $v eq ' ';
+
+        $vars{$k} = $v;
     }
 
     return \%vars;
@@ -296,7 +311,9 @@ sub packVars
         $$varsref{$_} = " " unless $$varsref{$_};
     }
 
-    return join("&", map( $_."=".escape($$varsref{$_}), keys %$varsre
+f) );
+    # current format version: 01
+    return join("&", "==01",
+        map( escape($_)."=".escape($$varsref{$_}), keys %$varsref) );
 }
 
 #####################################################################
+########
[download]

I think I've placed more emphasis (or at least tried to) on readability and reuse than what you had suggested. I'm guessing it will be about as fast as the existing code. If you are ok with this, I'll try it out on the test server.

I think I've placed more emphasis (or at least tried to) on readability and reuse than what you had suggested.

Yeah style wise I agree. But there was a point to my use of that type of packing scheme. Its a lot faster and the resulting text is smaller than the current one and it renders the text less illegibile in raw form. Just avoiding the overhead of the escape call is worthy IMO. Also the scheme I used can be made a litte more dynamic and self documenting by a more verbose rewrite. I think its worth finding out the full requirements of this (ie possible charset restrictions) and getting a fast light implementation done once and for all. Settings are used a _lot_, shaving time on them is very worthwhile IMO.

But i definately like this a lot more than the earlier ideas. One last thing, can I suggest that you use == at the start of the string? What you have there is a possible valid start of a vars. '=00&' could come from { ''=>'00','1'=>'1' } for instance. I suggest '==00&' so that its impossible to occur under the current codebase.



---
demerphq


First they ignore you, then they laugh at you, then they fight you, then you win.
-- Gandhi

Flux8