jolly-good-toolbelt / sphinx_gherkindoc

A tool to convert Gherkin into Sphinx documentation
https://jolly-good-toolbelt.github.io/sphinx_gherkindoc/
11 stars 10 forks source link

Add option to integrate background steps right into the scenario #11

Closed rbcasperson closed 5 years ago

rbcasperson commented 5 years ago

I really like this!

Screen Shot 2019-07-19 at 4 13 32 PM

There is no Background section in the docs, but each scenario gets the background steps integrated in. I think the italicized background steps subtly let you know that these are common steps in the feature.

dgou commented 5 years ago

Yeah.... I'm a fan of subtle, but italics doesn't convey background to me. I am still thinking I might like " (Background)" appended to the step text, but I would have to see it to know :-)

brolewis commented 5 years ago

I'm going to go ahead and say I don't like the idea of prepending (Background). The intention is for these to be "human readable" and that for me would be visually distracting.

brolewis commented 5 years ago

:lewis: The opinions expressed above are simply mine. I wanted to make sure it was clear I was only speaking for myself. </:lewis:>

dgou commented 5 years ago

I agree prepending would be but i was suggesting appending where it would be off to the right. I think once #10 lands we can start seeing actual examples

brolewis commented 5 years ago

10 has landed so we can test it once @rbcasperson handles the pyproject.toml merge conflicts

dgou commented 5 years ago

Screenshot from 2019-07-19 23-09-19

I pulled this PR and built our docs. It's subtle, but to me reading this it's not clear why that step is italicized

dgou commented 5 years ago

Screenshot from 2019-07-19 23-11-04

I like this slightly better as it tells me that step is from the background.

dgou commented 5 years ago

Screenshot from 2019-07-19 23-25-32

Another option to italicize both the step and the parenthetical note.

Still have other ideas to play with this weekend.

I think this overall idea is a good one, and it might make sense to merge with PR 3 since that provides slightly different presentation that might mesh better with any of these options and/or suggest improvements. Maybe from PR 3, change the leading character on the GWT steps to "B" or something to indicate the step came from the background?

brolewis commented 5 years ago

Yeah, I think I like the visuals in #3 so we can get that one done fist.

rbcasperson commented 5 years ago

I think there is also the question of whether or not we should care if it came from the background? If you're using this flag, then you care about making each scenario look like a complete scenario independently. I think at that point you've given up on caring what is in the background vs. only the scenario. Background is a gherkin term. By integrating the background steps into the scenario, we can make it so you can read the scenario without knowing that gherkin term, which would be useful for showing it to people who aren't used Gherkin.

I don't like appending B or Background. Really what I'm arguing is to drop the italics and treat a background step exactly the same as any other step.

dgou commented 5 years ago

So my overnight insomnia thoughts on this will hopefully still make sense by light of day. I see where you are coming from, but I have a slightly different goal. I'm not sure I think that taking the Gherkin-ness away from the reader is a good thing, it's already pretty dang simple. However, I don't feel super strongly about it and I think there is a way you can get your goal and I can get mine. (My goal is to keep the reader from having to scroll back up to the top to remind themselves what the Background steps actually are, so I like having some distinction between background steps and the scenario's specific steps.)

What if we have two switches:

  1. keep --integrate-background and all it does is arrange for the background steps to be included (and elides the background section).
  2. add --background-format which takes a format string with one substitution. The default can be "{}", but for my case I could use: "{} *(Background)*" (or "*{} (Background)*" or ... )

??

brolewis commented 5 years ago

I think I like Doug's insomniac brain's idea!

rbcasperson commented 5 years ago

Yeah that's an interesting thought. Why not?

bradsbrown commented 5 years ago

Weighing in from the "cheap seats" here to say:

dgou commented 5 years ago

I've also been noodling on this. Feature Backgrounds "set the stage" for all the scenarios in a feature file, so they're supposed to be just stuff you know when you read through the feature file. While I like having the background copied into each scenario (with formatting to show it is background), I also feel that the default should be to present the feature file "as is" when it comes to where things are... so leaving the background as it looks in the source. Further insomniac thoughts this morning is remembering (I don't recall where) that when you have one feature file with a bunch of features, but only some of them have common steps that should be in a background, that's a "gherkin smell" that you need to split up the feature file. ((hopefully this makes sense when someone else reads it))

rbcasperson commented 5 years ago

Sounds like there's agreement on how this should be. I'll get this PR updated when I can. In the meantime, #3 is ready to go.

rbcasperson commented 5 years ago

Assigning this out since I think this can merge as is with a follow up PR to incorporate the --background-step-format flag. Let me know if they have to go together.

rbcasperson commented 5 years ago

So I went ahead and implemented that related flag anyway. Kind of late here, so no guarantees my own late night brain didn't do anything odd. PR reviews FTW!

The code, docs building scripts, tests, and HTML for the built docs should all be sufficient to demonstrate, document, and test this new feature.