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

Encourage users to customize the permitted word list in RSpec/ContextWording (was: Add before and after to default list) #861

Merged
merged 1 commit into from Jan 28, 2020

Conversation

elebow
Copy link
Contributor

@elebow elebow commented Jan 16, 2020

Add before and after to the RSpec/ContextWording default list. This allows for more expressive context descriptions.

Example:

# bad
context 'record not yet saved'

# good
context 'when the record is not yet saved'

# even better
context 'before the record is saved'

Also add items that are mentioned in the documentation but not included in the default config: if, unless, for.

on-behalf-of: @Cofense oss@cofense.com


Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the changelog if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

  • Added the new cop to config/default.yml.
  • The cop documents examples of good and bad code.
  • The tests assert both that bad code is reported and that good code is not reported.

@pirj
Copy link
Member

pirj commented Jan 16, 2020

Can you please provide a full example group for which 'before the record is saved' is a good title?

Just for the reference, similar request: #757

We're trying to stick to https://rspec.rubystyle.guide/#context-descriptions (https://github.com/rubocop-hq/rspec-style-guide#context-descriptions) for defaults.
You are welcome to contribute to those guidelines as well.

@elebow
Copy link
Contributor Author

elebow commented Jan 16, 2020

Thanks for the pointers to other PRs. I've opened rubocop/rspec-style-guide#108 which seems like a better place for the discussion.


An example where the suggested change improves readability.

Suppose an application has a user creation flow with several stages:

  1. A user creates an account
  2. The app sends an email for the user to confirm their address
  3. After email is confirmed, an admin must manually approve the new user

A high-level behavioral spec may want to examine the situation at intermediate points in the process.

describe 'User creation' do
  context 'when the user has not yet confirmed their email address' do
  end

  context 'when the user has confirmed their email address' do
    # alternatively, 'when an admin has not yet approved the user'
  end

  context 'when an admin has approved the user' do
  end
end

This is adequate, but it does not do a good job communicating the flow of time—The groups are not alternative states, but a sequence of states. Words like before and after let the descriptions convey that more naturally:

describe 'User creation' do
  context 'before the user confirms their email address' do
  end

  context 'after the user has confirmed their email address' do
    # alternatively, 'before an admin approves the user'
  end

  context 'after an admin has approved the user' do
  end
end

@pirj
Copy link
Member

pirj commented Jan 17, 2020

Basing on the discussion in #757, and keeping in mind that:

do you think there's a way to suggest the users relax this cop in a way that lets them move forward with their style of choice without making it completely useless by default because it's too permissive?

@elebow
Copy link
Contributor Author

elebow commented Jan 23, 2020

In light of rubocop/rspec-style-guide#108, a documentation-only change to the ContextWording cop seems in order. Changes pushed to this PR.

Context descriptions are written in freeform human language, so it is impossible to foresee all acceptable cases. This change adds text to the documentation encouraging users to configure the list of permitted prefixes to meet their needs.

This change may also resolve #510.

@elebow elebow changed the title Add before and after to RSpec/ContextWording default list Encourage users to customize the permitted word list in RSpec/ContextWording (was: Add before and after to default list) Jan 23, 2020
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

lib/rubocop/cop/rspec/context_wording.rb Outdated Show resolved Hide resolved
# the configuration to meet project needs. Other acceptable prefixes may
# include `if`, `unless`, `for`, `before`, `after`, or `during`.
#
# @see https://rspec.rubystyle.guide/#context-descriptions
# @see https://github.com/reachlocal/rspec-style-guide#context-descriptions
Copy link
Member

Choose a reason for hiding this comment

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

If you're up for making another correction, the namespace has changed from reachlocal to rubocop-hq.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's the same content as https://rspec.rubystyle.guide, maybe it's better to have only one?

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the github.com link, since the rubystyle.guide link has the same content in a better form. Changes pushed.

lib/rubocop/cop/rspec/context_wording.rb Show resolved Hide resolved
Context descriptions are written in freeform human language, so it is
impossible to foresee all acceptable cases. Add text to the
documentation encouraging users to configure the list of permitted
prefixes to meet their needs.

on-behalf-of: @Cofense <oss@cofense.com>
@Darhazer Darhazer merged commit 218d2a0 into rubocop:master Jan 28, 2020
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