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

Modified regular expressions for support wrapping in tags. #1195

Merged
merged 3 commits into from
Oct 25, 2013

Conversation

jarosluv
Copy link
Contributor

If I want to wrap day or month in html-tags

<b>DD</b> <i>MMMM</i>

old regular expressions don't work.

@icambron
Copy link
Member

Can you explain this a bit more?

@Xotic750
Copy link
Contributor

This doesn't look such a good idea to me, regex matching of HTML elements is notoriously error prone. I would say it would be better for the end user to perform their own stripping before passing tokens to moment, they are then responsible for their mistakes.

@jarosluv
Copy link
Contributor Author

The patch for Russian language. This regular expression is only checks what grammatical case must be used.
For example,

DD MMMM

puts

17 октября  # 17 october in accusative case

But if I need to use html-tags, escaped symbols or anything else

DD <b>MMMM</b>

old regular expression doesn't work, because puts date in nominative case (but must be accusative case)

17 <b>октябрь</b> # 17 october in nominative case

My patch can fix this problem.

DD <b>MMMM</b>

puts

17 <b>октября</b>  # 17 october in accusative case

@icambron
Copy link
Member

@Xotic750 - I believe @jarosluv is referring to formatting, not parsing, e.g.:

> moment().lang('ru').format('DD [<b>]MMMM[</b>]')
'21 <b>октябрь</b>'

Where Russian uses special rules to determine how to format the month.

But I do have some concerns:

  1. Is .* really correct? For example, what if there are a bunch of other tokens between the two? I don't know Russian, but in English, you could conceivably say stuff like "The 14th day of January" or other variants; is the fix robust in cases like this? I don't want this to turn into a mess of special cases or HTML-specific code.
  2. The fix would need tests.

@Xotic750
Copy link
Contributor

@icambron Yes, I realised my mistake after the further explanation and a look at the change made. It was simply my bad assumption from reading the initial post. :)

@icambron
Copy link
Member

@Xotic750 👍

@jarosluv
Copy link
Contributor Author

Sorry for confusion. Yes, I really referring to formatting.

Let's analyze this situation step by step.

What is required?
When you want to display the day and month name in Russian language (in "DD MMMM" format), month name must be in the correct grammatical case.

What we have now?
Original regular expression (RE) checks only a single situation, when the day and month name are separated by space:

> moment().lang('ru').format('DD MMMM')
"22 октября" // true, accusative case

But if we want to add any symbols between DD and MMMM, RE doesn't work, because puts month name in nominative case (but must be accusative case):

> moment().lang('ru').format('DD [<b>]MMMM[</b>]')
"22 <b>октябрь</b>" // false, nominative case

What I suggest?
I suggest adding support for tags, escaped symbols, etc.

The best solution.
I belive my fix (.*) quite dirty, it solves the problems of the original RE, but it can create problems for any other situations.
I have reflected on this topic and came to the conclusion that we only need to add support escaped characters for original RE.
If there are any unescaped characters except space between DD and MMMM, will be displayed nominative case, otherwise accusative. This will be the best solution for most situations.

> moment().lang('ru').format('DD MMMM')
"22 октября" // true, accusative case

> moment().lang('ru').format('DD [<b>]MMMM[</b>]')
"22 <b>октября</b>" // true, accusative case

> moment().lang('ru').format('DD[-й день] MMMM')
"22-й день октября" // true, accusative case

> moment().lang('ru').format('DD[-й день], месяц MMMM')
"22-й день, месяц октябрь" // true, nominative case

> moment().lang('ru').format('DD, MMMM')
"22, октябрь" // true, nominative case

There is new RE:

/D[oD]?(\[[^\[\]]*\]|\s+)+MMMM?/

@icambron
Copy link
Member

@jarosluv That's fine with me. Want to edit the pull request accordingly and add some tests?

@jarosluv
Copy link
Contributor Author

@icambron Sure :)

…ses in Russian language. Escaped symbols doesn't affect the correct format of grammatical cases in pattern 'D[oD]? MMMM?'.
@jarosluv
Copy link
Contributor Author

@icambron Is it alright?

@icambron
Copy link
Member

Yup, looks good!

icambron added a commit that referenced this pull request Oct 25, 2013
Modified regular expressions for support wrapping in tags.
@icambron icambron merged commit 9da828d into moment:develop Oct 25, 2013
@Menelion
Copy link

@jarosluv, @icambron, I assume that should be modified for every (Slavic?) language that has different cases depending on environment? I'm talking about Ukrainian in particular now.

@icambron
Copy link
Member

@Oire I suppose that makes sense

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.

4 participants