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

Add more prefixes for RSpec/ContextWording #87

Merged
merged 1 commit into from Jun 4, 2020

Conversation

benthorner
Copy link
Contributor

@benthorner benthorner commented Jun 2, 2020

No description provided.

Copy link
Contributor

@issyl0 issyl0 left a comment

Choose a reason for hiding this comment

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

I dunno. I was always taught that contexts are for "when" and specifying the "when" makes sense.

@kevindew
Copy link
Member

kevindew commented Jun 2, 2020

I guess the cost of removing this is we remove anything that tries to maintain that contexts are authored with a when/with which is popularised (for example betterspecs). However this contrasts with our challenge to implement this since we have 100's of nested contexts that use context "and" which can't be autocorrected and will be painful?

Since we allow nested contexts we probably need something other than when/with if we want to continue with nested contexts. Otherwise things that read like: "FindByPath.find when there is a route with a prefix match and the path is a prefix at the same segment finds nothing as this isn't supported" become "FindByPath.find when there is a route with a prefix match when the path is a prefix at the same segment finds nothing as this isn't supported" which looks a bit wrong.

Since we have the RSpec\ExampleWording cop enabled this cop isn't without precedent (I did wonder if it was the only one that didn't anything like this). Would an idea be to add "and" and "but" to the list of words a context can include to make it more conventional with how we've written our code originally?

@benthorner benthorner changed the title Disable RSpec/ContextWording Add more prefixes for RSpec/ContextWording Jun 3, 2020
@benthorner
Copy link
Contributor Author

@kevindew that's a good suggestion, and helps with my overall goal of making this config easier to adopt by apps. I've amended the PR to permit "and" and "but".

Could you take another look?

Copy link
Member

@kevindew kevindew 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 to me 👍

Have you tried it out on Asset Manager to see if it fits in well? I know rubocop-rspec allows a few more words than these but I don't think they're commonly used in our apps.

@benthorner
Copy link
Contributor Author

Have you tried it out on Asset Manager to see if it fits in well?

Yes, this will allow for an improvement to my existing PR to sync the config.

I know rubocop-rspec allows a few more words than these

I don't think so: the defaults are quite conservative.

@benthorner benthorner merged commit 312157b into master Jun 4, 2020
@benthorner benthorner deleted the disable-context-wording branch June 4, 2020 09:57
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