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

Misc doc updates #1461

Merged
merged 19 commits into from Apr 9, 2014
Merged

Misc doc updates #1461

merged 19 commits into from Apr 9, 2014

Conversation

cupakromer
Copy link
Member

Starting to do a complete review of all the Relish docs. I often find myself using these daily or trying to point users to them. I've created a list of things that need updating / adding. This is the first pass on the core docs fixing mostly minor issues.

  • Capitalize the first word of scenarios
  • Use back ticks for inline code and options consistently
  • Minor grammatical issues
  • Fix whitespace wraps as viewed on Relish (wrap to a general 80 cols)
  • Replace describe instances with RSpec.describe
  • Clean up any remaining instances of should syntax in example specs

@cupakromer cupakromer mentioned this pull request Mar 28, 2014
2 tasks
@cupakromer
Copy link
Member Author

I'm going to try wrapping this up after I get home from work later.

@cupakromer
Copy link
Member Author

I think this is ready for merging. Would appreciate a 2nd set of eyes for a sanity check.

@JonRowe
Copy link
Member

JonRowe commented Mar 31, 2014

Needs a rebase...

@cupakromer
Copy link
Member Author

One note about the whitespace. After doing a last pass to see how things would render on Relish, there are a few cases where what shows up on relish, doesn't match nicely with the 80 col. breaks.

This seems to stem from Relish using a non-fixed width font. Which can apparently have a drastic affect on line lengths in some situations. The only full solution seems to be to remove the custom line breaks and let the paragraphs continue on and force your editor to perform the wrapping.

either pass a `-rrspec/autorun` CLI option when invoking `ruby`, or add
a `require 'rspec/autorun'` to one or more of your spec files.
Generally, life is simpler if you just use the `rspec` command. If you must use
the `ruby` command, however, you'll need to require `"rspec/autorun"`. You can
Copy link
Member

Choose a reason for hiding this comment

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

I know this is historical, but I'm not sure why we need " around rspec/autorun here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I think that should be: require "rspec/autorun"

@cupakromer
Copy link
Member Author

Change made and rebased against master.

either pass a `-rrspec/autorun` CLI option when invoking `ruby`, or add
a `require 'rspec/autorun'` to one or more of your spec files.
Generally, life is simpler if you just use the `rspec` command. If you must use
the `ruby` command, however, you'll need to `require "rspec/autorun"`. You can
Copy link
Member

Choose a reason for hiding this comment

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

the original phrasing was correct here, (require rspec/autorun) as the mechanism of requiring is specified later.

@JonRowe
Copy link
Member

JonRowe commented Mar 31, 2014

Good stuff so far, left some feedback, my major nitpick is at the moment there's plenty of code in Feature/Scenario headings not backticked. (I only commented on a few); I know it doesn't highlight properly but we're currently inconsistent about it and feel we should go full backtick. Developers will mostly know what we mean by them.

@myronmarston
Copy link
Member

@cupakromer -- thanks for tackling this! It'd be good to add some configuration that enforces certain things about the code in the scenario examples, such as RSpec.describe, the expect syntax, etc. We've done this for rspec-expectations and the same thing should work here:

https://github.com/rspec/rspec-expectations/blob/master/features/support/disallow_certain_apis.rb

@cupakromer
Copy link
Member Author

I'll make another pass after work to take care of the backticks for code in the feature and scenario names. I hadn't changed it previously, aside from it not highlighting, as most of the features didn't have them. Only later did I notice a few which did. I'll be sure to standardize on this going forward.

@myronmarston that's really cool. I'll take a crack at it when I get home from work.

@cupakromer
Copy link
Member Author

@JonRowe @myronmarston Sorry for the delay, work + house issues have left little free time recently. I put together a small public relish project to demo the different syntax highlighting currently: https://relishapp.com/cupakromer/code-test/docs/testing-code-blocks

You can see the feature I used in this gist: https://gist.github.com/cupakromer/9934026

@myronmarston
Copy link
Member

Hmmm, looks like you're right. Not sure what I was thinking of. Carry on :).

@JonRowe
Copy link
Member

JonRowe commented Apr 2, 2014

So only works if it's inline then..., that's ok, I think I still prefer over 4 spaces...

@cupakromer
Copy link
Member Author

@JonRowe we use 4-spaces all over the specs. Just to confirm, we're going to standardize on always using ``` for code blocks in feature descriptions with no indenting?

@cupakromer
Copy link
Member Author

Ok finishing this tonight. Made all requested changes. Also, on a happy note, it seems Relish is working out the issues with ```syntax 😄 I'll continue to work with them on that and all the edge cases.

Two things left:

- Explicitly specify the code block format for Relish
- Adjust wording to keep related topics together
Adjust the spec file names to match the description.
Relish supports using triple backticks for code blocks in markdown. This
is primarily used in feature descriptions. Relish has a few issues with
supporting naming the syntax with triple backticks. We'll standardize on
this method while Relish works the remaining issues out.
- Bold notes, warnings, etc. titles
- Move convention into a sub-heading
@cupakromer
Copy link
Member Author

This should be all done now.

@@ -239,7 +239,7 @@ passed to `it_should_behave_like`.

See [features/example\_groups/shared\_example\_group.feature](http://github.com/rspec/rspec-core/blob/master/features/example_groups/shared_example_group.feature) for more information.

NOTICE: The including example groups no longer have access to any of the
**NOTICE:** The including example groups no longer have access to any of the
Copy link
Member

Choose a reason for hiding this comment

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

including or included?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe "including" is correct. It seems to be referring to the example group which calls it_behaves_like, which is "including" the shared examples.

Copy link
Member

Choose a reason for hiding this comment

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

How about "An example group including shared examples no longer has access to any of the methods, hooks or state defined inside the shared group." I think it reads slightly better...

Copy link
Member Author

Choose a reason for hiding this comment

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

🔧 Fixed.

Copy link
Member Author

Choose a reason for hiding this comment

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

For this file, I didn't actually go over it initially. I just used a regex replace to handle the initial bolding of the notice heading.

@JonRowe
Copy link
Member

JonRowe commented Apr 6, 2014

Looking good, few more queries :)


rspec.expose_dsl_globally = false

rspec.include DisallowOneLinerShould unless ENV['ALLOW_ONELINER_SHOULD']
Copy link
Member

Choose a reason for hiding this comment

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

Why would we want to allow this? (Bearing in mind this is a global setting, not per spec setting)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is in response to @myronmarston comment about adding standard checks to the code (see: #1461 (comment)).

It's not really global for cucumber specs. This file is added before each feature when it is setup. It is easy to allow the one liner should syntax by tagging the spec with @oneliner-should; rspec-expectations already does this. We can add more tags when necessary too.

Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I don't care very much about enforcing one-liner syntax in rspec-core's cukes because we generally don't use it in rspec-core's cukes except for the ones that are demonstrating the one-liner syntax (where we need to allow both forms anywhere).

rspec-expectations uses the one-liner syntax extensively because it helps us show many usages of each matcher without much extra noise of surrounding code, so we wanted to enforce it there.

But given that you've already set this up, this is great :).

- Remove one-liner `should` and `should_not`
- Allow for full override of the customizations
Adjust whitespace for feature description based on space taken by added
tags.
@cupakromer
Copy link
Member Author

@myronmarston After considering more about the extended checks in the Before block (here 49ca43e) I decided to remove them. The way cucumber loads Ruby files would require a bit of adjustment to make a change I'd be happy with. I'm good with this setup for now.

@myronmarston
Copy link
Member

Sounds good. I haven't been following this PR in much depth but what I've seen looks good to me and I trust you and @JonRowe so if you think it's ready, merge away.

cupakromer added a commit that referenced this pull request Apr 9, 2014
@cupakromer cupakromer merged commit 4984a2b into master Apr 9, 2014
@cupakromer cupakromer deleted the misc-doc-updates branch April 10, 2014 02:03
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

3 participants