Skip to content

[Ruby 2.5] Add spec for String#start_with? supporting regexp - #551

Merged
eregon merged 2 commits into
ruby:masterfrom
nilsding:feature-13712/string-start_with-regexp
Jan 10, 2018
Merged

[Ruby 2.5] Add spec for String#start_with? supporting regexp#551
eregon merged 2 commits into
ruby:masterfrom
nilsding:feature-13712/string-start_with-regexp

Conversation

@nilsding

Copy link
Copy Markdown
Contributor

This PR adds a spec for Feature #13712: String#start_with? with regexp.

@marcandremarcandre self-assigned this Dec 13, 2017
@marcandre

Copy link
Copy Markdown
Member

This is a good start.
I don't think the spec for case sensitivity is needed. We can't test all the possible regexp scenario here, and there's nothing special about case sensitivity and start_with?. Maybe it would be good instead to check what happens if the regexp uses ^ and/or $.
Also, is Regexp.last_match ($1, $2, ...) kept intact or not.
Would you like to amend your PR?

@eregon

Copy link
Copy Markdown
Member

@nilsding Could you improve the PR based on @marcandre's review?

@nilsding

Copy link
Copy Markdown
ContributorAuthor

Oh, of course! Kinda forgot about that, the past few weeks have been a bit stressful for me.

@nilsding
nilsdingforce-pushed the feature-13712/string-start_with-regexp branch from bcf41f5 to 8a78b79CompareDecember 28, 2017 20:25
regexp1 = /^\d{2}/
regexp2 = /\d{2}$/
"12test".start_with?(regexp1).should be_true
"test12".start_with?(regexp1).should be_false

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Do you think it would make sense to add another expectation for something along "chunky\n12bacon" (would return false in this case)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess "chunky\n12bacon".start_with?(/12/).should be_false might be a good idea, just to be sure that it doesn't test for a line starting with, but the whole string starting with

@nilsdingnilsdingDec 28, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

good idea indeed, I'll add that one

@nilsding
nilsdingforce-pushed the feature-13712/string-start_with-regexp branch from 8a78b79 to c527983CompareDecember 28, 2017 21:19

@eregoneregon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks great, thank you for the new spec!

@eregon
eregon merged commit 10c1011 into ruby:masterJan 10, 2018
Sign up for freeto 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.

3 participants

@nilsding@marcandre@eregon