Codecov Report

Merging #966 into dev will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               dev      #966   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          172       175    +3     
  Lines         1547      1641   +94     
  Branches       328       359   +31     
=========================================
+ Hits          1547      1641   +94

Impacted Files	Coverage Δ
src/locale/la.js	`100.00% <100.00%> (ø)`
src/locale/br.js	`100.00% <0.00%> (ø)`
src/locale/ca.js	`100.00% <0.00%> (ø)`
src/locale/de.js	`100.00% <0.00%> (ø)`
src/locale/sr.js	`100.00% <0.00%> (ø)`
src/locale/en-nz.js	`100.00% <0.00%> (ø)`
src/locale/sr-cyrl.js	`100.00% <0.00%> (ø)`
src/plugin/duration/index.js	`100.00% <0.00%> (ø)`
src/plugin/localeData/index.js	`100.00% <0.00%> (ø)`
src/plugin/objectSupport/index.js	`100.00% <0.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2e1e426...40d4d4e. Read the comment docs.

Jul 23 '20 05:07 codecov[bot]

There's no need to create a unit test compared with moment.js's result. Just compare with normal string, please.

Jul 29 '20 02:07 iamkun

I have updated this PR with additional tests. I tried to create tests for everything in la.js, but the coverage output still says it is not 100 %. Could you explain to me why is it so? Thanks.

node node_modules/jest/bin/jest.js test/locale/la.test.js --coverage=true

 PASS  test/locale/la.test.js
  ✓ Latin: Test relative time (106ms)
  ✓ Latin: Test ordinal numbers (3ms)
  ✓ minimal weekday names (2ms)
  Latin: Test output of month names:
    ✓ in genitive case if day of month is output (11ms)
    ✓ in nominative if day of month is not output (3ms)
    ✓ also as a shortcut (2ms)
  Latin: Test weeday names:
    ✓ full weekday names (2ms)
    ✓ short weekday names (1ms)
    ✓ minimal weekday names (2ms)

----------------------------|----------|----------|----------|----------|-------------------|
File                        |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------------------------|----------|----------|----------|----------|-------------------|
All files                   |    85.54 |    59.39 |    75.71 |    86.84 |                   |
 src                        |    88.36 |    62.73 |    78.43 |    89.35 |                   |
  constant.js               |      100 |      100 |      100 |      100 |                   |
  index.js                  |    85.79 |    61.87 |    77.78 |    86.55 |... 57,364,368,399 |
  utils.js                  |    96.15 |    68.18 |    83.33 |      100 |       14,24,28,43 |
 src/locale                 |    95.24 |    83.33 |      100 |    95.24 |                   |
  en.js                     |      100 |      100 |      100 |      100 |                   |
  la.js                     |    95.24 |    83.33 |      100 |    95.24 |                20 |
 src/plugin/advancedFormat  |    45.83 |     8.33 |       75 |    45.83 |                   |
  index.js                  |    45.83 |     8.33 |       75 |    45.83 |... 33,35,37,39,41 |
 src/plugin/localizedFormat |    84.62 |       40 |       60 |      100 |                   |
  index.js                  |    84.62 |       40 |       60 |      100 |       15,16,17,20 |
 src/plugin/relativeTime    |    88.57 |    85.71 |    57.14 |    90.63 |                   |
  index.js                  |    88.57 |    85.71 |    57.14 |    90.63 |          57,75,78 |
----------------------------|----------|----------|----------|----------|-------------------|
Handlebars: Access has been denied to resolve the property "statements" because it is not an "own property" of its parent.
You can add a runtime option to disable the check or this warning:
See https://handlebarsjs.com/api-reference/runtime-options.html#options-to-control-prototype-access for details
Handlebars: Access has been denied to resolve the property "branches" because it is not an "own property" of its parent.
You can add a runtime option to disable the check or this warning:
See https://handlebarsjs.com/api-reference/runtime-options.html#options-to-control-prototype-access for details
Handlebars: Access has been denied to resolve the property "functions" because it is not an "own property" of its parent.
You can add a runtime option to disable the check or this warning:
See https://handlebarsjs.com/api-reference/runtime-options.html#options-to-control-prototype-access for details
Handlebars: Access has been denied to resolve the property "lines" because it is not an "own property" of its parent.
You can add a runtime option to disable the check or this warning:
See https://handlebarsjs.com/api-reference/runtime-options.html#options-to-control-prototype-access for details
Test Suites: 1 passed, 1 total
Tests:       9 passed, 9 total
Snapshots:   0 total
Time:        2.716s
Ran all test suites matching /test\/locale\/la.test.js/i.

Oct 01 '20 19:10 tukusejssirs

@iamkun, okay, I’ve found the cause of the non-100-% issue: NaN in romanise() function was the culprit, which was ignored by eslint (as it is disabled for the function), but it was still caught by Codecov.

Anyway, now the code is working, passes all tests (npm test + Travis + Codecov), therefore from my point of view this PR is mergable.

As a sidenote/question: could I open a new issue for dom/month/year romanisation that would be done within dayjs? I personally don’t plan implementing it, but it might be a nice feature that is not found in any other date/time implementation, but it is still used by real humans (e.g. in Latin and Hungarian). Only one of dom/month/year could be romanised at a time and it could be implemented as a dayjs flag/option, like romanise = 'year' (possible options: false, year, month, dom; default: false).

Oct 02 '20 08:10 tukusejssirs

Added a commit with small update: now la.js is fully complying the ESLint rules (there is no eslint-disable.

Oct 05 '20 12:10 tukusejssirs

Is there anything that blocks this PR from being merged? Is there anything else I need to do before this PR could be merged?

Oct 15 '20 11:10 tukusejssirs

Cool, thx. Waiting for native speaker review ( 0 / 1 )

Oct 15 '20 13:10 iamkun

@iamkun, I see.

@igneus, could you review this PR from the linguistic point of view? We (the romcal dev/users) need this merged, because otherwise dayjs with default to English locale.

Some notes to guide to in the review to do it real quick:

When I started the work on this PR, I asked some people at LatinDiscussion.com to proof the Latin used in the code. You might want to read it.
The la.js file contains the following:
- on line 4: month names in genitive (used in dates, where there is at least a day of month and month, while month is written out);
- on line 5: month names in nominative (used otherwise);
- on lines 17-31:
  - there is function romanise() that converts Arabic numerals to Roman numeral;
  - note that currently, it is only used in ordinal numbers (line 42; see below: on line 42), because a romanisation module won’t get into the dayjs codebase (for more info, see this comment or even the whole conversation in #961);
- on line 35: full weekday names as used in the Missale Romanum (ed. typ. tertia);
- on line 36: shorter weekday names (Saturday and Sunday are not shortened, for weekdays I used feria + number);
- on line 37: short/abbreviated weekday names;
- on line 39: abbreviated month names;
- on line 42:
  - the format of ordinal numbers;
  - it is only used when the day of month (D) is followed by o (lowercase letter o); this is how dayjs uses it (I can’t do anything about it);
  - I’ve read somewhere that it is followed by ᵒ (Modifier Letter Small O; U+1D52), however it is not obligatory;
- on lines 45-57: relative time:
  - as currently, I don’t know of a way to output all forms in all grammer cases, therefore I decided to include only the forms in nominative;
  - the strings are quite easy; in case you need there English translation, see e.g. en-gb.js (L13-26).

PS—@igneus, sorry if I the above description is too detailed, however, I wish you’d find some time to review this PR ASAP. However, I don’t want to hurry you nor push you into anything. :smiley:

Oct 15 '20 15:10 tukusejssirs

I looked through the code and tests and so far haven't found any obvious errors.

Thanks, @igneus, for such a prompt review! 👍

But it has to be clearly said that the authority of my approval is quite low. I'm just a theologian, no Latin expert, and without any training in "modern Latinity".

IMHO, still better a review than no review. :wink:

Anyway, I aim this PR to add Ecclesiastical Latin, i.e. one used in the current (port Vatican II) liturgical books.

Oct 15 '20 21:10 tukusejssirs

Add initial version of Latin locale

Codecov Report