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.

Sign up for GitHub

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

Jump to bottom

[Str] add grarpheme support #74

Merged
merged 1 commit into from
Nov 2, 2020
Merged

[Str] add grarpheme support #74

merged 1 commit into from
Nov 2, 2020

Conversation

azjezz
Copy link
Owner

@azjezz azjezz commented Oct 3, 2020 •
edited
Loading

closes #62
refs #63

@azjezz azjezz added Status: In Progress This issue is being worked on, and has someone assigned. Type: Enhancement Most issues will probably ask for additions or changes. labels Oct 3, 2020
@azjezz azjezz added this to the v1.0.0 milestone Oct 3, 2020
@azjezz azjezz self-assigned this Oct 3, 2020
@azjezz azjezz force-pushed the feature/str-grapheme branch from ac837f8 to bdfca5e Compare October 3, 2020 18:18
@azjezz azjezz requested a review from veewee October 3, 2020 18:18
@azjezz azjezz added Status: Review Needed The issue has a PR attached to it which needs to be reviewed. and removed Status: In Progress This issue is being worked on, and has someone assigned. labels Oct 3, 2020
@azjezz azjezz force-pushed the feature/str-grapheme branch from bdfca5e to 05b6b3c Compare October 3, 2020 18:20
@azjezz azjezz mentioned this pull request Oct 3, 2020
Closed
azjezz
azjezz commented Oct 3, 2020
View reviewed changes
src/Psl/Str/Grapheme/length.php Outdated
*/
function length(string $string): int
{
return (int) grapheme_strlen($string);
Copy link
Owner Author

Choose a reason for hiding this comment

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

I think there's a case where grapheme_strlen returns null, not sure how to handle that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The docs state it always returns an int though:
https://www.php.net/manual/en/function.grapheme-strlen.php

(We could keep it - just to be sure :) )

Copy link
Owner Author

Choose a reason for hiding this comment

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

I learned to never belive the documentation 😆

see:

not sure how to handle these cases ...

Copy link
Owner Author

Choose a reason for hiding this comment

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

I added invariant violation checks.

@azjezz azjezz force-pushed the feature/str-grapheme branch 2 times, most recently from a1d030c to 9ec32cb Compare October 4, 2020 17:13
@azjezz
Copy link
Owner Author

azjezz commented Oct 6, 2020

/cc @veewee If you think we can move ahead with this, let's merge it.

@azjezz azjezz force-pushed the feature/str-grapheme branch from 9ec32cb to 9f0bb13 Compare October 7, 2020 07:53
@coveralls
Copy link

coveralls commented Oct 7, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 715

  • 74 of 74 (100.0%) changed or added relevant lines in 14 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 704: 0.0%
Covered Lines: 1847
Relevant Lines: 1847
💛 - Coveralls

@azjezz azjezz force-pushed the feature/str-grapheme branch from 9f0bb13 to 33fd4e2 Compare October 7, 2020 08:01
veewee
veewee previously approved these changes Nov 2, 2020
View reviewed changes
Copy link
Collaborator

@veewee veewee left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Besides the conflicts, it Looks OK to me...

src/Psl/Str/Grapheme/length.php Outdated
*/
function length(string $string): int
{
return (int) grapheme_strlen($string);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The docs state it always returns an int though:
https://www.php.net/manual/en/function.grapheme-strlen.php

(We could keep it - just to be sure :) )

@azjezz azjezz force-pushed the feature/str-grapheme branch 3 times, most recently from 3b0c554 to f322e91 Compare November 2, 2020 17:19
@azjezz azjezz changed the base branch from develop to 0.1.x November 2, 2020 17:32
@azjezz azjezz dismissed veewee’s stale review November 2, 2020 17:32

The base branch was changed.

@azjezz azjezz force-pushed the feature/str-grapheme branch from f322e91 to bba7f5f Compare November 2, 2020 17:32
@azjezz azjezz merged commit e1b45c2 into 0.1.x Nov 2, 2020
@azjezz azjezz deleted the feature/str-grapheme branch December 21, 2020 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@veewee veewee veewee left review comments

Assignees

@azjezz azjezz

Labels
hacktoberfest-accepted Status: Review Needed The issue has a PR attached to it which needs to be reviewed. Type: Enhancement Most issues will probably ask for additions or changes.
Projects
None yet
Milestone
0.1.0
Development

Successfully merging this pull request may close these issues.

[Str] Grapheme support
3 participants
@azjezz @coveralls @veewee