Indicative docstrings #71

Closed
wants to merge 9 commits into
from

8 participants

@rentalcustard

This makes generated docstrings come out in the indicative rather than reading e.g. 'should do X', so that we don't get specdoc that mixes and matches when using the indicative in explicit docstrings.

It introduces a new method, docstrings, on matchers which returns a hash for negative and positive docstrings, but it also tries to avoid putting too much burden on the authors of matchers by a) providing a default implementation for matchers defined via the dsl and b) falling back on 'should/should_not #{description}' when the docstrings method isn't present.

The tests and features pass, although I'm aware of some tests in rspec-core (e.g. http://relishapp.com/rspec/rspec-core/v/2-6-rc/dir/command-line/line-number-option) which will break (in easily fixable ways) as a result of applying these changes.

I'm aware that this is a pretty invasive change and I wouldn't be surprised to hear that it breaks something else as currently implemented, but I wanted to throw up a pull request/issue to solicit thoughts/discussion as much as anything else.

@dchelimsky
RSpec member

@mortice - nice! My instinct is the hash keys should be :should and :should_not instead of :positive and :negative (aligns with method names). WDYT?

@rentalcustard

@dchelimsky I'm not sure - it feels a bit jarring to me to have strings which explicitly avoid 'should/should not' as the values for keys with those names. I don't really have strong feelings about it though...

@dchelimsky
RSpec member

I'll buy that. I'm trying to get the 2.6.0 release out this week and this is a big enough change that I don't want to rush it into this release (especially since there's already been 3 release candidates). I'm definitely planning to add it to 2.7, though. Thanks!

@rentalcustard

Great! Feel free to @ me if/when it breaks stuff on application :)

@myronmarston
RSpec member

Great work! This is a really nice change.

The API of having a method return a hash w/ two entries seems a bit strange to me--I can't think of any other ruby API I've worked with that has used that. What would you think about changing it to something like this?

def docstring_for_should
  "is empty"
end

def docstring_for_should_not
  "is not empty"
end

This aligns nicely w/ the rest of the matcher API, I think. It does include should and should_not in the names, which you mentioned wanting to avoid, but to me it reads differently here--it doesn't imply it should have "should" in the strings, it just is explicitly defining the doc string for should or should_not.

I also wonder if some people still prefer the it "should do something" docstring style. I don't, but for those that do, they might not like this change. A config option could potentially help here, but I'm not sure I'm in favor of that, either. I'm not sure what the right answer is.

@dchelimsky
RSpec member

I'm definitely in favor of a config option, defaulting to the old format for now - then default to the new format in 3.0.

@rentalcustard

Thanks for the feedback Myron.

The hash return is definitely odd, come to think of it. My initial thought was to try and bring the description into the hash as well, trying to avoid adding lots of methods onto matchers. But on second thoughts, it makes little difference whether we add 1 or 2 new methods, and I like your suggested names in this context too.

I was wondering about a config option myself. I'll hack something together there - presumably that means an addition to RSpec core which would end up being upstream of this? Or is there a way of avoiding that dependency?

@rentalcustard

See above for part 1. I've tried to add the option via the add_setting method, doing this in a file which gets required on rspec-expectations load:

RSpec.configure do |config|
  config.add_setting :indicative_docstrings, :default => false
end

But when I try to override this setting for a cucumber scenario to demonstrate it, i.e. by including this at the top of a 'Given a file named 'X.rb' with:

RSpec.configure do |config|
  config.indicative_docstrings = true
end

I get "undefined method `indicative_docstrings=' for #<RSpec:Configuration" when I run cucumber.

I must be doing something wrong here - any chance of some pointers?

@rentalcustard

As a side note, I'm not sure about indicative_docstrings as the name for the config value... any suggestions?

@dchelimsky
RSpec member

How about

config.generated_docstring_format = :suggestive # default
config.generated_docstring_format = :indicative
@justinko

Great work! This is a really nice change.

What @myronmarston said.

@rentalcustard

Added config option with above. I went with modal vs indicative because 'suggestive' has... erm... other implications in my mind. :)

@rentalcustard

Sorry for the obnoxious force push there, my mistake.

@rentalcustard

I hate to be a pain, but I'd love to see this merged and there hasn't been much activity here. Anything I can do to help it on its way?

@dchelimsky
RSpec member

I'm planning to merge it, but it won't be released until 2.7, which is probably a month out. It's a big patch and I'll need time to review it, etc, and I'm at RailsConf this week. Feel free to ping again if you don't see any activity in a couple of weeks.

@rentalcustard

Thanks David. I assumed that was the case but just wanted to make sure you didn't think anything major was missing. :)

@Mange

What's up with this one? I was surprised it wasn't part of 2.7.

@dchelimsky
RSpec member

Just lost track of it with other priorities. Getting ready to drop 2.8 in the next few days and my focus is on documentation, so this definitely won't be part of that, but I'll try to get it in after that.

@dchelimsky
RSpec member

Another problem I just realized is that github won't let me assign a milestone to this pull request as it does for issues :( Weak!

@dchelimsky
RSpec member

It's listed under the 2.9 milestone

@Mange

Great. Cheers!

@soulcutter
RSpec member

This is awesome! Thanks, @mortice

@dchelimsky
RSpec member

I'm going to go ahead and take care of merging, given that it sat around so long. Belated thanks for all the work, @mortice.

@rentalcustard

It was a pleasure, although I'd completely forgotten about it! Thanks for taking on the (presumably very hairy by this stage) merge, @dchelimsky.

@dchelimsky dchelimsky added a commit that referenced this pull request Sep 14, 2012
@rentalcustard rentalcustard indicative docstrings
This was a pull request (#71) from Tom Stuart that I didn't merge until several
months later, so I had to address some merge conflicts. In addition, the
original intent was to offer the end user a configuration option for the
"voice" of the message (e.g. "should be x" vs "is x"), but I made it check if
the matcher responds to new methods docstring_for_should or
docstring_for_should_not and use those. This way all the internal matchers do
the right thing, but external matchers will still work without rspec trying to
convert the mode of it's docstring.
2f0d105
@dchelimsky
RSpec member

@mortice very hairy, indeed! I ended up basically copying line by line from the diff here. I put you in as the author, although I made some changes. I pushed it to a separate branch because I'd like your approval to merge it to master since it has your name but there are some differences in approach.

@rentalcustard

I'm sorry I'm so behind in taking a look at this. I've had a look at 2f0d105 and am happy to have my name put to it as co-author or whatever makes most sense to you. Essentially, you could throw out all my code, rewrite and just credit me in the changelog with the original idea/implementation and I'd be happy; this is your project!

Let me know if there's anything I can do to help, though. I've just finished emigrating so I finally have some spare time to work on things.

@JonRowe
RSpec member

Did this eventually get merged elsewhere? Is it safe to close this issue or is it still awaiting work?

@myronmarston
RSpec member

@JonRowe, sadly, no this hasn't been merged. @dchelimsky rebased it and got it ready to merge in 2f0d105, but I had some concerns around introducing new parts of the matcher protocol that use the word should when we're moving away from that syntactically. (See my comment at 2f0d105#commitcomment-1856301)

As part of rspec 3, I plan to cleanup the matcher protocol w.r.t. should, and I think we should hold off on this until that shakes down.

@JonRowe
RSpec member

It occurs to me that it's shame there's no way to "snooze" or "pend" issues until a milestone (hint hint @github) but obviously this is best left to your discretion about the direction of the framework in general.

Off topic but personally I'm a fan of the should syntax and I'd be sad panda if it went away completely. Also what's the equivalent in the new expect syntax of its(:value) { should be_something } ?

@myronmarston
RSpec member

Off topic but personally I'm a fan of the should syntax and I'd be sad panda if it went away completely.

We have no plans to ever remove it completely. My plan is to disable it by default in a future release (likely RSpec 3.0, but that's open to discussion), but even then, it'll be one configuration line away from being available.

Also what's the equivalent in the new expect syntax of its(:value) { should be_something } ?

Rather than restate myself, I'll just point you to http://stackoverflow.com/questions/12260534/using-implicit-subject-with-expect-in-rspec-2-11/12266147#12266147 where I answered this :).

@samphippen
RSpec member

@mortice I'd be entirely up for remote pairing with you on this one if you want to carry it across the finish line for RSpec 3.0

@myronmarston
RSpec member

@samphippen -- IMO, this is gated on #270. @dchelimsky got this one rebased and ready to merge in 2f0d105 awhile ago but the sticking point was that we didn't want to add new methods to the API that use should in the name, given the direction RSpec has been going, and we couldn't come up with names we were happy with. While it's frustrating to let a PR linger, IMO it's best to hold off while we're unsure about naming new methods that are part of a protocol: once we add to it, it's very, very hard to change given the dependency that users now have on it.

All that said, since this PR was opened I've started to wonder/doubt it it's worth expanding the matcher protocol and adding additional infrastructure simply to change the grammatical "voice" of the generated doc strings. It seems very heavy weight for something that is, IMO, a minor feature of RSpec. I'm not sure if the tradeoff of the added infrastructure is worth adding for this feature. (Note: I'm not saying that I think it's not wroth adding this feature; I'm saying I'm on the fence.) Some RSpec users have rightly complained about how heavyweight RSpec is and this is the kind of stuff that contributes to that heavyweightness.

A couple alternate ideas:

  • We could just change the voice of the existing generated docstrings to the should-less form, w/o providing the config option infrastructure.
  • Users already configure the should vs expect syntax; could that be used to infer which doc string syntax they want to use?
@samphippen
RSpec member

After a quick chat on irc with @mortice I'm gonna close this:

[15:55:14] <mortice>     samphippen: i received the latest email about that PR while lying in bed in dubrovnik. Accordingly, I instantly forgot about it
[15:55:23] <samphippen>  I mean
[15:55:24] <samphippen>  wat
[15:56:17] <mortice>     i don't even care about it, no one bothers with -fdoc or doing anything useful with it anyway
[15:56:28] <mortice>     i just felt like hacking it together 2 years ago
[15:56:46] <samphippen>  ok
[15:56:52] <samphippen>  do you want to just close it out or  something?
[15:57:51] <mortice>     i don't care
[15:58:11] <mortice>     i like my PRs like I like my code: abandoned by me as soon as released
[15:58:15] <samphippen>  ok
[15:58:16] <mortice>     do what you like with it :)
[15:58:22] <samphippen>  I'll close it with a copy of this irc chat then
[15:58:56] <mortice>     seems legit
@samphippen samphippen closed this Sep 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment