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

French d'Words and l'Phrases capitalization #539

Merged
merged 33 commits into from
Aug 12, 2018
Merged

French d'Words and l'Phrases capitalization #539

merged 33 commits into from
Aug 12, 2018

Conversation

GlazerMann
Copy link
Collaborator

@GlazerMann GlazerMann commented Aug 10, 2018

No description provided.

@GlazerMann GlazerMann changed the title French d'Words and l'Phrases French d'Words and l'Phrases capitalization Aug 10, 2018
@codecov
Copy link

codecov bot commented Aug 10, 2018

Codecov Report

Merging #539 into master will decrease coverage by 2.72%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##             master   #539      +/-   ##
==========================================
- Coverage     75.73%    73%   -2.73%     
- Complexity     1447   1452       +5     
==========================================
  Files             9      9              
  Lines          2477   2497      +20     
==========================================
- Hits           1876   1823      -53     
- Misses          601    674      +73
Impacted Files Coverage Δ Complexity Δ
expandFns.php 95.55% <100%> (+0.39%) 0 <0> (ø) ⬇️
DOItools.php 93.7% <100%> (ø) 0 <0> (ø) ⬇️
Template.php 79.83% <0%> (-3.97%) 1300% <0%> (+5%)

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 e878e18...848ac58. Read the comment docs.

expandFns.php Outdated
@@ -221,6 +221,17 @@ function title_capitalization($in, $caps_after_punctuation) {
create_function('$matches', 'return mb_strtolower($matches[0]);'),
trim($new_case)
);

$new_case = preg_replace_callback("/ (L')([a-zÀ-ÿ]+)/", function($matches) { /** French l'Words **/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also need to match unicode alternatives to the apostrophe, i.e. ’ and kin?

expandFns.php Outdated
}, ' ' . $new_case);
$new_case = mb_ucfirst(trim($new_case));

$new_case = preg_replace_callback("/ (D')([a-zÀ-ÿ]+)/", function($matches) { /** French d'Words **/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use \b to match a word boundary, i.e. `\b(D'). And there's no need for brackets around the (D') / (L') as if there is a match, we don't need to refer to $matches to figure out what it will be!

Just to be thorough (-:
ms609 pushed a commit that referenced this pull request Aug 11, 2018
Add some more short words.
- d'un and d'une should be removed in #539.
@GlazerMann
Copy link
Collaborator Author

@ms609 Thoughts? I made your tests even harder.

@GlazerMann
Copy link
Collaborator Author

@ms609 It actually works now

@@ -221,6 +221,20 @@ function title_capitalization($in, $caps_after_punctuation) {
create_function('$matches', 'return mb_strtolower($matches[0]);'),
trim($new_case)
);
/** French l'Words and d'Words **/
$new_case = preg_replace_callback(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a single function matching (\s[LD]) and replacing with mb_strtolower($matches[1])? This'll avoid duplication and keep the code leaner.
Does preg_replace_callback match all occurrences of the search expression?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes all

@ms609 ms609 merged commit 193f0ef into ms609:master Aug 12, 2018
@GlazerMann GlazerMann deleted the patch-33 branch August 15, 2018 16:27
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.

None yet

2 participants