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

Projects
None yet
4 participants
@jarosluv
Contributor

jarosluv commented Oct 16, 2013

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

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

old regular expressions don't work.

@icambron

This comment has been minimized.

Member

icambron commented Oct 17, 2013

Can you explain this a bit more?

@Xotic750

This comment has been minimized.

Contributor

Xotic750 commented Oct 17, 2013

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

This comment has been minimized.

Contributor

jarosluv commented Oct 17, 2013

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

This comment has been minimized.

Member

icambron commented Oct 21, 2013

@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

This comment has been minimized.

Contributor

Xotic750 commented Oct 21, 2013

@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

This comment has been minimized.

Member

icambron commented Oct 21, 2013

@Xotic750 👍

@jarosluv

This comment has been minimized.

Contributor

jarosluv commented Oct 22, 2013

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

This comment has been minimized.

Member

icambron commented Oct 22, 2013

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

@jarosluv

This comment has been minimized.

Contributor

jarosluv commented Oct 22, 2013

@icambron Sure :)

jarosluv added some commits Oct 16, 2013

Modified regular expressions for correct formatting of grammatical ca…
…ses in Russian language. Escaped symbols doesn't affect the correct format of grammatical cases in pattern 'D[oD]? MMMM?'.
@jarosluv

This comment has been minimized.

Contributor

jarosluv commented Oct 25, 2013

@icambron Is it alright?

@icambron

This comment has been minimized.

Member

icambron commented Oct 25, 2013

Yup, looks good!

icambron added a commit that referenced this pull request Oct 25, 2013

Merge pull request #1195 from jarosluv/develop
Modified regular expressions for support wrapping in tags.

@icambron icambron merged commit 9da828d into moment:develop Oct 25, 2013

1 check passed

default The Travis CI build passed
Details
@Menelion

This comment has been minimized.

Menelion commented Oct 25, 2013

@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

This comment has been minimized.

Member

icambron commented Oct 25, 2013

@Oire I suppose that makes sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment