Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Case sensitivity in [deploy.variables] section #48

Closed
rdunklau opened this issue Oct 15, 2012 · 18 comments
Closed

Case sensitivity in [deploy.variables] section #48

rdunklau opened this issue Oct 15, 2012 · 18 comments
Labels

Comments

@rdunklau
Copy link

It looks like every key in a deploy.variables section is converted to lowercase, thus unusable as a psql variable if spelled in all caps in the scripts.

This limitation does not exists with the overriding command line argument, --set, which preserves the case.

@theory
Copy link
Collaborator

theory commented Oct 16, 2012

Oy, this is because Config::GitLike lowercases the keys.

@alexmv, I wrote this method in a subclass to return all the key/value pairs in a section as a hash reference:

sub get_section {
    my ( $self, %p ) = @_;
    $self->load unless $self->is_loaded;
    my $section = $p{section} // '';
    my $data    = $self->data;
    return {
        map { $_ => $data->{"$section.$_"} }
        grep { s{^\Q$section.\E([^.]+)$}{$1} } keys %{$data}
    };
}

Works fine, except that data stores all the keys in lowercase. I understand that you do this because, according to Git, they are case-insensitive, but I wonder if we could find a way to preserve the case. Thoughts?

@alexmv
Copy link

alexmv commented Oct 16, 2012

Yeah, there should probably be a Config::GitLike flag as to if you care about case sensitivity. What you actually care about is case preservation, not case sensitivity, though, right?

@theory
Copy link
Collaborator

theory commented Oct 16, 2012

Yes, the key lookup should still be case-insensitive. I just want to be able to fetch the keys in their original case. I can't think of way to do it offhand that doesn't require duplication of some kind (e.g., a hash mapping the lowercase keys to the preserved case keys where they differ).

@theory
Copy link
Collaborator

theory commented Oct 16, 2012

And it would have to be post-merge, too. So if ~/config has foo.bar.Baz and ./config has foo.bar.baz, the key would need to be foo.bar.baz.

@alexmv
Copy link

alexmv commented Oct 16, 2012

Yeah, that's the only API I'm coming up with that's at all sensical. At a quick re-read of the code, I think it'd be vaguely irritating but not too bad to implement. I'm hosed too hosed to get to it myself this week, but can probably carve out some time next week.

@alexmv
Copy link

alexmv commented Oct 16, 2012

Oh, additional complication: multi-valued keys. If one files has foo.bar.Baz = troz and foo.bar.baz = zort, which one should get returned?

@theory
Copy link
Collaborator

theory commented Oct 16, 2012

As I said, whichever one wins. That is, the same one that has its value returned.

@alexmv
Copy link

alexmv commented Oct 16, 2012

I specified multi-valued keys, for which you get an array-ref back of all of the values set. I agree with you on non-multi-valued keys that the last one (which actually set the value) wins.

@theory
Copy link
Collaborator

theory commented Oct 16, 2012

Oh, multie-valued keys. Er, that’s a tough one. Maybe it causes an exception when one does not completely override the other and case preservation is required? Might be confusing, but I suspect it'd be pretty rare.

@theory
Copy link
Collaborator

theory commented Oct 17, 2012

Actually, if the goal is to preserve case in that context, they should both be provided.

@theory
Copy link
Collaborator

theory commented Nov 7, 2012

Hey @alexmv, what's the status on this? You had a chance to look into it any deeper?

@alexmv
Copy link

alexmv commented Nov 7, 2012

Apologies -- this had indeed slipped off my plate. Thanks for reminding me. Before I tag and release, can you check that the current master gives you an API that works for you?

@theory
Copy link
Collaborator

theory commented Nov 7, 2012

Thanks Alex. I think it works. Here's my revised get_section() method:

sub get_section {
    my ( $self, %p ) = @_;
    $self->load unless $self->is_loaded;
    my $section = $p{section} // '';
    my $data    = $self->data;
    return {
        map  {
            (my $k = $self->original_case("$section.$_")) =~ s{^\Q$section.\E([^.]+)$}{$1};
            $k => $data->{"$section.$_"}
        }
        grep { s{^\Q$section.\E([^.]+)$}{$1} } keys %{$data}
    };
}

Should I also be lcing $section?

@theory
Copy link
Collaborator

theory commented Nov 7, 2012

Oh, BTW, original_key might be a better name, no?

@alexmv
Copy link

alexmv commented Nov 7, 2012

original_key is indeed a better name -- I'll change that. You do also need to lc $section, as $self->datas keys are in canonical case, which always has the section lower-cased.

@theory
Copy link
Collaborator

theory commented Nov 7, 2012

Great, thanks. Updating the Sqitch code now.

@alexmv
Copy link

alexmv commented Nov 7, 2012

Version 1.10 is on its way to CPAN, if you want to update the dependency.

@theory
Copy link
Collaborator

theory commented Nov 7, 2012

Thanks!

@theory theory closed this as completed in d554342 Nov 7, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants