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

Spec: add expectations starts_with, ends_with #5881

Merged
merged 2 commits into from Apr 30, 2018
Merged

Spec: add expectations starts_with, ends_with #5881

merged 2 commits into from Apr 30, 2018

Conversation

kostya
Copy link
Contributor

@kostya kostya commented Mar 28, 2018

i need this many times.

@jhass
Copy link
Member

jhass commented Mar 29, 2018

Didn't make my mind up generally but it should be start_with and end_with respectively, since the proper English is "It should start with foo".

Sija
Sija reviewed Mar 29, 2018
View reviewed changes
src/spec/expectations.cr Outdated
@@ -200,6 +200,42 @@ module Spec
end
end

# :nodoc:
struct StartsWithExpectation(T)
Copy link
Contributor

Choose a reason for hiding this comment

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

StartsWithExpectation -> StartWithExpectation

Sija
Sija reviewed Mar 29, 2018
View reviewed changes
src/spec/expectations.cr Outdated
end

def failure_message(actual_value)
"Expected: #{actual_value.inspect}\nto start_with: #{@expected_value.inspect}"
Copy link
Contributor

Choose a reason for hiding this comment

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

start_with -> start with

RX14
RX14 requested changes Mar 29, 2018
View reviewed changes
src/spec/expectations.cr Outdated
@@ -266,6 +302,18 @@ module Spec
Spec::ContainExpectation.new(expected)
end

# Creates an `Expectation` that passes if actual start_with *expected* (`.starts_with?`).
Copy link
Contributor

Choose a reason for hiding this comment

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

"starts with expected (.starts_with?)"

src/spec/expectations.cr Outdated
Spec::StartWithExpectation.new(expected)
end

# Creates an `Expectation` that passes if actual end_with *expected* (`.ends_with?`).
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@RX14
Copy link
Contributor

RX14 commented Mar 29, 2018

I'm a bit wary about having a "slippery slope" towards duplicating all collection/string methods as spec expectations, we shouldn't have too many.

@kostya
Copy link
Contributor Author

kostya commented Mar 29, 2018

there is not so many, for strings useful only contain, start_with, end_with and match

asterite
asterite approved these changes Apr 28, 2018
View reviewed changes
Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

I agree with @kostya here, starts_with?, ends_with? and match (for which we already have a matcher) are very common operations in tests. It's not that bad to add just this one spec matcher.

@asterite
Copy link
Member

@kostya By the way, string_spec.cr in the std specs could use this new matcher.

@kostya
Copy link
Contributor Author

kostya commented Apr 29, 2018

i not found in string_spec where to replace it.

@asterite
Copy link
Member

@kostya https://github.com/crystal-lang/crystal/blob/master/spec/std/string_spec.cr#L943-L968

@straight-shoota
Copy link
Member

@asterite These are specs explicitly for String#starts_with?. Using the expectation here would clearly compromise these specs. It hides their true purpose and might even be completely wrong if StartWithExpectation wouldn't use String#starts_with? internally.

@asterite
Copy link
Member

Okay... I don't see it as that terrible, but it's also not strictly necessary.

@asterite
Copy link
Member

So we just need the approval of @RX14

@RX14 RX14 added this to the Next milestone Apr 30, 2018
@RX14 RX14 merged commit 3e88a62 into crystal-lang:master Apr 30, 2018
chris-huxtable pushed a commit to chris-huxtable/crystal that referenced this pull request Jun 6, 2018
@kostya @chris-huxtable
Spec: add expectations starts_with, ends_with (crystal-lang#5881)
88df23c 
* Spec: add expectations starts_with, ends_with

* fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sija Sija Sija left review comments

@asterite asterite asterite approved these changes

@RX14 RX14 RX14 approved these changes

Assignees
No one assigned
Labels
kind:feature topic:stdlib:specs topic:stdlib
Projects
None yet
Milestone
0.25.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@kostya @jhass @RX14 @asterite @straight-shoota @Sija