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

Support case insensitive names #7

Merged
merged 2 commits into from Aug 15, 2013
Merged

Support case insensitive names #7

merged 2 commits into from Aug 15, 2013

Conversation

ghost
Copy link

@ghost ghost commented Jul 9, 2013

@szabgab
Copy link
Collaborator

szabgab commented Jul 10, 2013

If I am not mistaken I got two pull request for the same change from two different Github users. (Pull 6 and 7)
Which one should I use (and credit)?
In any cases, please add a unit test where both the old behaviour and the new behaviour is tested.

Then in the code, instead of going over all the keys of the hash with a regex, you could create another hash $self->{'syntaxes_map'} where the keys are the lower case keys from the $self->{'syntaxes'} and the values are the original values. Then the actual code would be just a lookup in the new hash. $self->{'syntaxes_map'} can be created once.

@ghost
Copy link
Author

ghost commented Jul 10, 2013

Use whoever gets the test case done first, first in best dressed.

A hash of lower case keys for another hash, that is always in memory, seems more expensive that a loop that only gets called if you are using the new behaviour.

@szabgab
Copy link
Collaborator

szabgab commented Jul 10, 2013

You can fill the extra hash the first time it it needed.

@ghost
Copy link
Author

ghost commented Jul 10, 2013

It only gets used once per session, so it's hanging around for the rest of your actions for no reason.

@szabgab
Copy link
Collaborator

szabgab commented Jul 10, 2013

ok, then don't us a temp hash but instead of the regex just compare the lc version of the two strings.

add test cases.
szabgab added a commit that referenced this pull request Aug 15, 2013
Support case insensitive names
@szabgab szabgab merged commit 9fa9156 into manwar:master Aug 15, 2013
@szabgab
Copy link
Collaborator

szabgab commented Aug 15, 2013

thanks and sorry for the delay

@ghost
Copy link
Author

ghost commented May 28, 2014

HI, is it possible to do a new release with this patch? I'm trying to avoid carrying the patch on Windows :}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant