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

Optional msgtag format #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ivilata
Copy link

@ivilata ivilata commented Feb 15, 2021

This makes the __MSG_*__ format of message tags optional, accepting ^([\w@]+)$ as well. See #1.

Please note that the documentation file for Localizer.js has been updated manually in a different commit, so it's easy to drop if a true update via JSDoc is preferred instead.

If not using that format, the tag still needs to stick to the characters
allowed for message names.
This has been done manually, but trying to reproduce the output from JSDoc.
@rugk
Copy link
Member

rugk commented Feb 17, 2021

Hi @ivilata,
thanks for your first contribution to this project! 🎉 👍 🏅
I hope you'll like this project and enjoy hacking on it… 😃

Interesting solution of just adjusting the fallback, I had rather though of a solution trying to parse the string with the existing RegEx and falling back to the existing string, if that does not work. Would not that be easier?

Also to simplify the RegEx, this could be made:
https://regex101.com/r/W4qrqb/1
I.e. making the optional parts optional and not specifying an alternative inside of it…

Though, also for fail-safety, I'd prefer just to fallback to the existing string in case the RegEx-extraction fails. What do you think?

@rugk
Copy link
Member

rugk commented Oct 19, 2022

Also please adjust the Readme as that syntax is now optional…

@ivilata If you need any pointers/help or have questions, feel free to ask.

Mention the __MSG_*__ syntax for backwards compatibility.
@ivilata
Copy link
Author

ivilata commented Oct 20, 2022

Thanks @rugk for the heads up (and for the ping, as I clearly forgot to reply to the previous message).

Commit be32fef should update the readme as requested. Regarding the simplified RegEx, I think that it would also match something like translationName__, so I'll leave it as is for the moment. If you want to accept these too, or whatever other string which doesn't match the RegEx (your fallback suggestion), I guess it's up to you, but somehow out of this PR's scope. 😉

Thanks again and cheers!

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.

2 participants