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

Do not remove text if key isn't in messages.json #2

Merged
merged 1 commit into from Feb 25, 2018

Conversation

alexeyr
Copy link
Contributor

@alexeyr alexeyr commented Feb 24, 2018

If used with non-existent key (e.g. because of a typo), keep the __MSG_... in place, so it can be noticed and fixed easier.

@piroor
Copy link
Owner

piroor commented Feb 25, 2018

Thanks, but I think aMatched is better than aString, for input like "__MSG_XXXXX__: ". Could you update the PR?

@alexeyr
Copy link
Contributor Author

alexeyr commented Feb 25, 2018

aString is intentional because XXXXX could be something meaningful in target language but with different meaning (https://en.wikipedia.org/wiki/False_friend), __MSG_XXXXX__ couldn't.

@piroor
Copy link
Owner

piroor commented Feb 25, 2018

Sorry for my less informational comment. I mean that aString duplicates extra part of the input. For example:

(function (aString) {
  return aString.replace(/__MSG_(.+?)__/g, function(aMatched) {
    return aString;
  });
})('__MSG_XXXXX__:');
// => "__MSG_XXXXX__::" (because aString is "__MSG_XXXXX__:")

(function (aString) {
  return aString.replace(/__MSG_(.+?)__/g, function(aMatched) {
    return aMatched;
  });
})('__MSG_XXXXX__:');
// => "__MSG_XXXXX__:"  (because aMatched is "__MSG_XXXXX__", not "XXXXX")

If used with non-existent key (e.g. because of a typo), keep the `__MSG_...` in place, so it can be noticed and fixed easier.
@alexeyr
Copy link
Contributor Author

alexeyr commented Feb 25, 2018

Got it, fixed.

@piroor piroor merged commit b094c17 into piroor:master Feb 25, 2018
@piroor
Copy link
Owner

piroor commented Feb 25, 2018

Thanks!

@alexeyr alexeyr deleted the patch-1 branch February 25, 2018 22:38
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

2 participants