Codecov Report

Merging #1049 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #1049   +/-   ##
=======================================
  Coverage   97.29%   97.29%           
=======================================
  Files          90       90           
  Lines        6987     6987           
=======================================
  Hits         6798     6798           
  Misses        189      189

Reviewable can ignore files that are marked as generated. Should we mark the UnicodeTable.inc as such?

Review status: all files reviewed at latest revision, all discussions resolved.

update_unicode.py, line 450 at r1 (raw file):

      else:
        raise RuntimeError( 'Cannot handle Extended_Pictographic combined with '
                            '{} property'.format( break_property ) )

Can this exception really be raised?

cpp/ycm/Character.cpp, line 101 at r1 (raw file):

      case BreakProperty::E_MODIFIER:
      case BreakProperty::GLUE_AFTER_ZWJ:
      case BreakProperty::E_BASE_GAZ:

No ExtPict here?

cpp/ycm/Word.cpp, line 183 at r1 (raw file):

            within_emoji_modifier = false;
            break;
          // Rule GB11: do not break within emoji modifier sequences of emoji

What happened to rule 10?

Comments from Reviewable

Reviewable can ignore files that are marked as generated. Should we mark the UnicodeTable.inc as such?

I am not sure there is much point as Rewiewable is somewhat already ignoring the file since it considers it's a binary file.

Reviewed 9 of 11 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

update_unicode.py, line 450 at r1 (raw file):

It may happen in a future version of the Unicode standard if a new Emoji code point has the Extended_Pictographic property and a breaking property different from Other. In that case, we would need to add a new breaking property.

cpp/ycm/Character.cpp, line 101 at r1 (raw file):

No because ExtPict code points are derived from Other which are valid base characters (contrarily to Prepend, Extend, and SpacingMark which are not characters on their own).

cpp/ycm/CodePoint.h, line 46 at r2 (raw file):

  LV                 = 12,
  LVT                = 13,
  EXTPICT            = 18

Renamed ExtPict to EXTPICT for consistency with other property names.

cpp/ycm/Word.cpp, line 183 at r1 (raw file):

The rule has been removed.

Comments from Reviewable

Fair enough.

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

I have essentially reviewed this syntactically. The whole unicode stuff is so far beyond my understanding at this point that I'm not really adding any value as a reviewer. based on that.

Review status: all files reviewed at latest revision, 1 unresolved discussion.

update_unicode.py, line 313 at r2 (raw file):

  nb_code_points = 0
  emoji_data = defaultdict( list )
  for line in data:

for the record, this is all magic to me. As an offline tool, I haven't spent the time to really grok and review it properly. If you think that's useful, then I can, just shout.

Comments from Reviewable

Reviewed 8 of 11 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

Comments from Reviewable

Sure, I can't really expect you to go through the Unicode specs to make sure the changes in this PR are sound. The important part is to see that new tests has been added for that version of Unicode (extracted from NormalizationTest.txt and GraphemeBreakTest.txt) and that those tests are passing. Still, thanks for taking your time to review this (even if only syntactically).

@zzbot r+

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

update_unicode.py, line 313 at r2 (raw file):

No, I am confident this part of the script is doing what it's supposed to i.e. extracting the properties of the Emoji code points from emoji-data.txt and storing them into the dictionary: code point value -> list of properties.

Comments from Reviewable

📌 Commit ae44dc1 has been approved by micbou

⌛ Testing commit ae44dc1 with merge 7b587ef...

💔 Test failed - status-travis

I cancelled the build because the core version wasn't bumped anymore.

@zzbot r+

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

📌 Commit 683cb5e has been approved by micbou

⌛ Testing commit 683cb5e with merge 81cec7f...

☀️ Test successful - status-appveyor, status-travis
Approved by: micbou
Pushing 81cec7f to master...