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

Use is_expected.to instead of should #215

Closed
wants to merge 1 commit into from
Closed

Use is_expected.to instead of should #215

wants to merge 1 commit into from

Conversation

JoelQ
Copy link
Contributor

@JoelQ JoelQ commented Aug 6, 2014

Even though we've switched to the expect syntax, we've still been using
should for one-liners with an implicit subject. RSpec 3 introduced
is_expected.to as an alternative. Myron Marston explains:

Some users have expressed confusion about how this should relates to the
expect syntax and if you can continue using it. It will continue to be
available in RSpec 3 (again, regardless of your syntax configuration), but
we’ve also added an alternate API that is a bit more consistent with the
expect syntax

From: New API for
one-liners

Even though we've switched to the `expect` syntax, we've still been using
`should` for one-liners with an implicit subject. RSpec 3 introduced
`is_expected.to` as an alternative. Myron Marston explains:

> Some users have expressed confusion about how this should relates to the
> expect syntax and if you can continue using it. It will continue to be
> available in RSpec 3 (again, regardless of your syntax configuration), but
> we’ve also added an alternate API that is a bit more consistent with the
> expect syntax

From: [New API for
one-liners](http://myronmars.to/n/dev-blog/2014/05/notable-changes-in-rspec-3#new_api_for_oneliners_)
@gabebw
Copy link
Contributor

gabebw commented Aug 6, 2014

This feels a little bit like change for the sake of change. I don't see any benefits beyond the fact that this has the word expect in it.

@calebhearth
Copy link
Contributor

Yeah, it's also a lot longer which I'm against.

@JoelQ
Copy link
Contributor Author

JoelQ commented Aug 6, 2014

@gabebw I think it's not so much because it has the word expect in it but because it doesn't have the word should which can be confusing since should means two different things in RSpec depending on the context.

@jessieay
Copy link
Contributor

jessieay commented Aug 6, 2014

I haven't been using is_expected.to for one liners yet, but I like it MUCH more than

it { expect(subject).to be_blah }

that I've seen on a few projects.

@mcmire
Copy link

mcmire commented Aug 7, 2014

I don't mind this change, though it would take getting used to. Personally, should isn't confusing to me, can you elaborate on why you think so? Does it mean two different things?

@sikachu
Copy link
Contributor

sikachu commented Aug 7, 2014

I actually thought that should for one-liner was deprecated. Seems like it's not?

@JoelQ
Copy link
Contributor Author

JoelQ commented Aug 7, 2014

@mcmire The following are not the same:

# monkey-patches Object, has been deprecated
# our guides state that we should use expect(subject).to here
it { subject.should be_true }

# doesn't monkey-patch Object, still available in RSpec3
# RSpec3 introduced is_expected.to as an alternative to this
it { should be_true }

@calebhearth
Copy link
Contributor

should as a one-liner isn't going away, and I still prefer it to more verbose alternatives.

@sikachu
Copy link
Contributor

sikachu commented Aug 7, 2014

Since should is not going away, I think let's keep should in one-liner.

@mcmire
Copy link

mcmire commented Aug 7, 2014

@JoelQ Gotcha, yeah, I see your point.

@jferris
Copy link
Member

jferris commented Sep 11, 2014

@JoelQ still interested in this? Any further opinions here?

@JoelQ
Copy link
Contributor Author

JoelQ commented Sep 11, 2014

I personally prefer is_expected.to but I'm fine sticking with should if the rest of the team prefers. I'd like to get a few more opinions on this before closing this PR though.

@nickrivadeneira
Copy link
Contributor

I prefer is_expected.to. I've used it on a recent project and it feels more consistent with expect(foo).to in other tests.

@drapergeek
Copy link
Contributor

I prefer should for one-liners.

@joshuaclayton
Copy link
Contributor

I've no opinion, I enjoy the expect-ish consistency and don't personally care about the additional characters per line, but I'm also not so convicted that I'd really argue if everyone wants to continue with should.

@derekprior
Copy link
Contributor

As best I can tell, the only reason this method was added was to keep people from continually asking what the one-line equivalent to expect is.

Should is shorter and reads better to me, but I don't feel super strongly about it.

@mcmire
Copy link

mcmire commented Sep 11, 2014

So leaving aside personal preference for a moment, what about people who are new to RSpec (i.e. apprentices), do we feel that is_expected.to makes more sense from that perspective given its similarity to expect(...)?

@iancanderson
Copy link
Contributor

I liked is_expected_to because it makes the language more consistent, which has the side-effect of making more sense to beginners, as @mcmire suggested

@derekprior
Copy link
Contributor

Good point, @mcmire. I don't think we should let "what's most newbie/community friendly" drive all of our guides but in the face of no real functional arguments for one or the other, I think that's an okay default.

How do we answer this question?

@robwise
Copy link

robwise commented Sep 11, 2014

A random person's 2 cents: I don't think it's necessary, and any "user-friendliness" is negated by the increased complexity of yet one more "no-no" to remember. Changing the RSpec config to expect the expect syntax should be good enough.

@JoelQ
Copy link
Contributor Author

JoelQ commented Sep 11, 2014

@robwise Currently the guides explicitly say to use should for one-liners. This PR suggests changing to using is_expected.to.

Are you suggesting that we do neither and have no guideline one what to use for one liners since both should and is_expected.to are valid when RSpec is configured with the expect syntax?

@BlakeWilliams
Copy link
Contributor

I'm slightly leaning towards should for one liners. is_expected.to looks a bit weird to me and is also a bit longer, which is something I don't want in a one liner.

@robwise
Copy link

robwise commented Sep 11, 2014

@JoelQ Well, completely eliminating the rule is actually an interesting idea, considering both are syntaxes are currently valid. Obviously, the invalid .should method syntax would still be disallowed.

But, no, my original suggestion was to keep the guide as-is, thereby allowing should one-liners. Changing the guide to require the is_expected.to syntax just doesn't make a whole bunch of sense to me for length reasons and unnecessary change.

@nickrivadeneira
Copy link
Contributor

Based on the documentation of RSpec, it feels like is_expected.to is the intended one-liner syntax when using the expect syntax whereas should might remain for legacy reasons.

  • is_expected is defined simply as expect(subject) and is designed for when you are using rspec-expectations with its newer expect-based syntax.
  • should was designed back when rspec-expectations only had a should-based syntax. However, it continues to be available and work even if the :should syntax is disabled (since that merely removes Object#should but this is RSpec::Core::ExampleGroup#should).

@robwise
Copy link

robwise commented Sep 11, 2014

More insight on that in this comment thread.

@jferris
Copy link
Member

jferris commented Oct 9, 2014

@JoelQ any updates here?

JoelQ added a commit that referenced this pull request Oct 9, 2014
Previously one-liners could be written in RSpec like:

  it { should validate_presence_of(:name) }

In RSpec 3, the `should` syntax was removed because it monkey-patched `Object`.
The one-liner syntax shown above was kept because its `should` did not
monkey-patch `Object`. To avoid confusion, RSpec 3 added an `expect`-style way
of writing one-liners:

  it { is_expected.to validate_presence_of(:name) }

According to Myron Marston:

> Some users have expressed confusion about how this should relates to the
> expect syntax and if you can continue using it. It will continue to be
> available in RSpec 3 (again, regardless of your syntax configuration), but
> we’ve also added an alternate API that is a bit more consistent with the
> expect syntax

From: [New API for
one-liners](http://myronmars.to/n/dev-blog/2014/05/notable-changes-in-rspec-3#new_api_for_oneliners_)

A proposal was made to switch to using the new `is_expected.to` syntaxin [this
pull request](#215). However, responses
were mixed. Given the lack of consensus one way or the other, let's remove the
guideline entirely and let each project decide which syntax to use.
JoelQ added a commit that referenced this pull request Oct 9, 2014
Previously one-liners could be written in RSpec like:

    it { should validate_presence_of(:name) }

In RSpec 3, the `should` syntax was removed because it monkey-patched `Object`.
The one-liner syntax shown above was kept because its `should` did not
monkey-patch `Object`. To avoid confusion, RSpec 3 added an `expect`-style way
of writing one-liners:

   it { is_expected.to validate_presence_of(:name) }

According to Myron Marston:

> Some users have expressed confusion about how this should relates to the
> expect syntax and if you can continue using it. It will continue to be
> available in RSpec 3 (again, regardless of your syntax configuration), but
> we’ve also added an alternate API that is a bit more consistent with the
> expect syntax

From: [New API for
one-liners](http://myronmars.to/n/dev-blog/2014/05/notable-changes-in-rspec-3#new_api_for_oneliners_)

A proposal was made to switch to using the new `is_expected.to` syntaxin [this
pull request](#215). However, responses
were mixed. Given the lack of consensus one way or the other, let's remove the
guideline entirely and let each project decide which syntax to use.
JoelQ added a commit that referenced this pull request Oct 9, 2014
Previously one-liners could be written in RSpec like:

    it { should validate_presence_of(:name) }

In RSpec 3, the `should` syntax was removed because it monkey-patched `Object`.
The one-liner syntax shown above was kept because its `should` did not
monkey-patch `Object`. To avoid confusion, RSpec 3 added an `expect`-style way
of writing one-liners:

    it { is_expected.to validate_presence_of(:name) }

According to Myron Marston:

> Some users have expressed confusion about how this should relates to the
> expect syntax and if you can continue using it. It will continue to be
> available in RSpec 3 (again, regardless of your syntax configuration), but
> we’ve also added an alternate API that is a bit more consistent with the
> expect syntax

From: [New API for
one-liners](http://myronmars.to/n/dev-blog/2014/05/notable-changes-in-rspec-3#new_api_for_oneliners_)

A proposal was made to switch to using the new `is_expected.to` syntaxin [this
pull request](#215). However, responses
were mixed. Given the lack of consensus one way or the other, let's remove the
guideline entirely and let each project decide which syntax to use.
@JoelQ
Copy link
Contributor Author

JoelQ commented Oct 9, 2014

Given the lack of consensus here, I'm going to close this PR. Feel free to reopen and continue the discussion. I'm also opening #234 to not have a guideline at all on RSpec one-liners.

@JoelQ JoelQ closed this Oct 9, 2014
@JoelQ JoelQ deleted the jq-is-expected branch January 14, 2015 16:32
JoelQ added a commit that referenced this pull request Jan 15, 2015
Previously one-liners could be written in RSpec like:

    it { should validate_presence_of(:name) }

In RSpec 3, the `should` syntax was removed because it monkey-patched `Object`.
The one-liner syntax shown above was kept because its `should` did not
monkey-patch `Object`. To avoid confusion, RSpec 3 added an `expect`-style way
of writing one-liners:

    it { is_expected.to validate_presence_of(:name) }

According to Myron Marston:

> Some users have expressed confusion about how this should relates to the
> expect syntax and if you can continue using it. It will continue to be
> available in RSpec 3 (again, regardless of your syntax configuration), but
> we’ve also added an alternate API that is a bit more consistent with the
> expect syntax

From: [New API for
one-liners](http://myronmars.to/n/dev-blog/2014/05/notable-changes-in-rspec-3#new_api_for_oneliners_)

A proposal was made to switch to using the new `is_expected.to` syntaxin [this
pull request](#215). However, responses
were mixed. Given the lack of consensus one way or the other, let's remove the
guideline entirely and let each project decide which syntax to use.
JoelQ added a commit that referenced this pull request Jan 16, 2015
Previously one-liners could be written in RSpec like:

    it { should validate_presence_of(:name) }

In RSpec 3, the `should` syntax was removed because it monkey-patched `Object`.
The one-liner syntax shown above was kept because its `should` did not
monkey-patch `Object`. To avoid confusion, RSpec 3 added an `expect`-style way
of writing one-liners:

    it { is_expected.to validate_presence_of(:name) }

According to Myron Marston:

> Some users have expressed confusion about how this should relates to the
> expect syntax and if you can continue using it. It will continue to be
> available in RSpec 3 (again, regardless of your syntax configuration), but
> we’ve also added an alternate API that is a bit more consistent with the
> expect syntax

From: [New API for
one-liners](http://myronmars.to/n/dev-blog/2014/05/notable-changes-in-rspec-3#new_api_for_oneliners_)

A proposal was made to switch to using the new `is_expected.to` syntaxin [this
pull request](#215). However, responses
were mixed. Given the lack of consensus one way or the other, let's remove the
guideline entirely and let each project decide which syntax to use.
@mcmire
Copy link

mcmire commented Mar 26, 2015

Do we want to revisit this? I'm finding more and more that people who leave issues on shoulda-matchers tend to prefer the is_expected syntax rather than the should syntax. I'm considering switching the README and documentation to use is_expected... but I'm hesitant to do this if we currently promote the use of should.

@JoelQ
Copy link
Contributor Author

JoelQ commented Mar 26, 2015

We don't promote either form. The guideline was removed in #234. I don't see a problem with converting the shoulda-matchers README to is_expected, although others may disagree.

@mcmire
Copy link

mcmire commented Mar 26, 2015

Oh yeah! It sure was. Cool, thanks.

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