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

Fixing filters with uppercase letters #132

Merged
merged 2 commits into from
Aug 1, 2017
Merged

Fixing filters with uppercase letters #132

merged 2 commits into from
Aug 1, 2017

Conversation

igalarzab
Copy link
Contributor

@igalarzab igalarzab commented Aug 1, 2017

Hi!

Filters that are using uppercase characters are broken as the lowercase() method is being executed to the name before registering them into the SimpleLibrary.

They are broken because in the fetch method the filter name is just lowercased before being compared with the existing ones, but not before adding them to the library, so what's happening is that:

  • The filters are registered with the camelcase name inside the library, ex: filterName.
  • The filters cannot be accessed, as the fetch method tries to find a filter named filtername.

There were two solutions:

  1. Lowercasing the filter before doing the put into the HashMap.
  2. Allowing filter names to have not just lowercase characters.

I chose the second option as I didn't see any reason for the restriction.

I added one test and the patch is backwards compatible, as filters with uppercase characters were not usable before.

@boulter boulter merged commit bc6de18 into HubSpot:master Aug 1, 2017
@igalarzab igalarzab deleted the fixing-camel-case-filters branch August 1, 2017 13:49
@igalarzab
Copy link
Contributor Author

igalarzab commented Aug 1, 2017

Hi @boulter,

that was fast! thanks for merging it :)

Any possibility of having a new release in mvn?

@jmp3833
Copy link
Contributor

jmp3833 commented Aug 1, 2017

Hi @igalarzab. Going to create a new release 2.2.4 in maven that includes these changes in addition to #133. I'll let you know when the new version is available 👍

@igalarzab
Copy link
Contributor Author

great, thanks @jmp3833 !

@jmp3833
Copy link
Contributor

jmp3833 commented Aug 1, 2017

No problem! Should be available in the central repository within the next couple of hours

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.

3 participants