Error for documentation #58

Closed
wants to merge 10 commits into
from

Projects

None yet

2 participants

@jamesalmond

I've written some examples for error_for documentation and filled out the narrative. Hope the style of the tests and narrative is ok! Any changes, let me know.

@jamesalmond

Improved docs for raise_error

@dchelimsky
Member

I merged your changes and then made some more. What you had there was definitely a great start, but I'm still exploring what is the best format for these docs, so I made some changes in the next commit: f5ddd17

At a high level, the whole point of the cuke scenarios is to present ... scenarios! It seemed like there was overlap between the narrative and the scenarios, so I shortened the narrative and restructured the scenarios.

In the narrative, you'll see that the code snippets are indented 4 spaces (2 more than in your commit) from the margin (which is also 2 spaces in). That's because the narrative is formatted with Markdown, which requires 4 spaces indentation for code snippets.

Let me know if you have any questions, and thanks so much for contributing to the documentation effort!

@jamesalmond

My mistake on the code indents, should have checked that.

Can I ask your thoughts behind why they changed so drastically?

I chose to put more examples in the narrative as other feature narratives used this. I wasn't sure it was correct as obviously the scenarios should be documenting the functionality so I can see why it has been reduced.

In terms of style, I was basing the scenarios on the ones that already exist, most of which provide examples of a failing test to assert that the behaviour actually works. My first error_for with a block example highlighted that you could write scenarios that pass even if the underlying code doesn't function correctly.

There's obviously a balance that needs to be found between documentation and a test suite, and it looks like the changes you made value the documentation side of them more (as they only assert on passing tests, a bug that made the matcher pass every assertion would mean the Cucumbers would still pass). Is this your intention or is there a specific reason you removed the failing examples? (I admit, I didn't think it was the nicest way of doing things) Wouldn't a failing example be a legitimate scenario for a testing framework?

One of the reasons I had a go at this in the first place was to try and get my head round how Cucumber as documentation would be written and what would make a good feature, so thanks for the previous feedback, and any more would be useful. Thanks!

@dchelimsky
Member

I played around with these for a bit and found that one example per scenario is easier to read as documentation. Agreed that we're missing a scenario with failing examples. One thing I almost did, but did not in the end, was add a single scenario at the bottom showing the error cases for all of the different forms. WDYT? That way it's there, but not in the way.

@jamesalmond

One example per-scenario sounds sensible and probably much clearer to read.

Adding the failing examples to one scenario at the end sounds like it could work. The scenario title would have to be clear and you could use something like "Then all the examples should fail" or "Then x examples should fail", depending on how you wanted to match it.

Also, how do you feel about documenting "should_not"? In most cases the intention of should_not is obvious, but in the error_for with message matchers, for example, it's probably not.

Also, (I'll stop pestering you soon!) you chose to use standard objects (e.g. Object.new.method) when some of the other features use classes defined in the example to trigger certain errors. Is that preferred?

I'd like to contribute more to the documentation, but is it worth letting the "style guidelines" settle down before I attempt some more? Is it better that there's more editorial control?

Thanks!

@dchelimsky
Member

Style guidelines won't settle down in a vacuum :) We need to have activity that draws attention to the different cukes to examine them more closely.

I think should_not should be documented. Quite often only a subset of the forms available for should make sense for should_not.

@kchien kchien pushed a commit to kchien/rspec-expectations that referenced this pull request Mar 7, 2014
@jamesalmond jamesalmond Improved docs for raise_error
- Closes #58.
1a60dff
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment