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

Fixes #9 #17

Closed
wants to merge 2 commits into from
Closed

Fixes #9 #17

wants to merge 2 commits into from

Conversation

briangriffey
Copy link

Now supports A-Za-z0-9_ keys

Keys allowed in the A-Z and 0-9 range

cleanup
@pforhan
Copy link
Contributor

pforhan commented Aug 4, 2015

Looks like this broke a test. You might wish to use Character.isJavaIdentifierPart() or Character.isLetterOrDigit() for something that reads a bit better. Of course, either could vastly expand the set of characters allowed, so they may not be a good idea.

Aside from that, this change also allows the first character to be _ or a number, where it explicitly required a lowercase char before.

@briangriffey
Copy link
Author

Oops, forgot to check that test in. I can do that when I get to work tomorrow.

Regarding the first char being a _ or number, I'm unsure why the restriction of "Keys start with lowercase letters followed by lowercase letters and underscores." was chosen. If I was adding capital letters it seems equally as arbitrary to start with a _. But I can change the code to whatever fits the project goals.

@@ -20,6 +20,7 @@
import android.content.res.Resources;
import android.text.SpannableStringBuilder;
import android.view.View;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra line. Square conventions only divide between imports and static imports

@loganj
Copy link
Collaborator

loganj commented Feb 4, 2016

#9 never really came with justification other than "why not?"

Closing unless #9 is reopened with reasoning.

@loganj loganj closed this Feb 4, 2016
@briangriffey
Copy link
Author

We needed it for our internal translation system. Also why not is always a good reason :-D

@loganj
Copy link
Collaborator

loganj commented Feb 4, 2016

It looks like keys will be case-sensitive though. {eXample} and {EXample} would be different. Is that necessary?

@briangriffey
Copy link
Author

I guess that's a consequence of allowing both upper case and lower case.

I'm actually fine with you punting this from the main repo. It's probably not a solution that everyone needs. We can merge into our fork when necessary.

@loganj
Copy link
Collaborator

loganj commented Feb 4, 2016

I guess that's a consequence of allowing both upper case and lower case.

Well, it could be case-insensitive.

@rocboronat
Copy link

Hi guys! We just had this issue with our team. A guy that is just introduced to Android was trying to replace {homeAddress}, and the library was launching a "Missing right bracket }".

The reason to accept non-lowercase chars is not a "Why not?". It's just "what people expects".

Thanks for the good work!

@rocboronat rocboronat mentioned this pull request Mar 10, 2016
@miquelo
Copy link

miquelo commented Mar 10, 2016

+1

@mohamedkomalo
Copy link

I have just faced the same problem by @rocboronat, it is the normal thing to expect especially when lots of people use camelCase, at least put a note in the readme that only lowercase letters is supported.

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

7 participants