Refactoring + new cops #3
Conversation
offence has been renamed to offense
|
@yujinakayama since you are very familiar with both rubocop and rspec it would be very nice of you if you could take a look into these changes, and let me know what you think. |
|
@bbatsov maybe you could take a look at these changes - especially the new cops. Its the extension of the existing gem to add new rspec rubocop cops as discussed in 1087. At the moment I prefer prefixing the cops with |
|
@nevir sorry for so many changes - I just came from one idea for a cop to the other. Feel free to let me know what you think and what needs to be changed from your point of view. |
|
@geniou Namespacing is certainly problematic in the moment. I think we need to add some proper support in RuboCop for that, since I'd hate for people to have to prefix class names like in PHP4 :-). @jonas054 any ideas regarding this? Guess we can have cop sections in the config or something similar... |
|
@bbatsov I totally agree with you and looking forward to have a better solution. |
| def on_block(node) | ||
| method, _args, _body = *node | ||
| _receiver, method_name, _object = *method | ||
| @in_describe = true if method_name == :describe |
There was a problem hiding this comment.
There are some aliases and shared examples/contexts.
https://github.com/yujinakayama/transpec/blob/master/lib/transpec/rspec_dsl.rb
|
Going through this now, but this looks fantastic so far! Thanks for splitting the file cops apart too :) |
| module Rubocop | ||
| module Cop | ||
| # Helper methods for top level describe cops | ||
| module TopLevelDescribe |
There was a problem hiding this comment.
Not a fan of this being under the Cop namespace - that should be reserved for cop classes only, IMO. Rubocop::RSpec::TopLevelDescribe maybe?
...then again, there are examples of modules under Cop in the main repo - so, either way.
There was a problem hiding this comment.
I followed the the structure in the main repo - but this does not mean we have to do it the same way.
|
Just nitpicks from me. See geniou#1 (feel free to merge that and this PR once you're happy w/ it) |
|
@bbatsov I have an idea regarding name spaces. Basically we include one level of name space in the cop names, so This is relatively easy. I have a solution on branch. The hard part is to make the transition painless for the users. I don't know how to do that. |
|
@jonas054 Would it be an idea, for the transition, to keep the current behavior for all config values without the "namespace" and just print a warning, that this will be deprecated? In this case the rubocop-rspec gem can use the prefix, and the users have time to migrate. |
|
We're pre 1.0. They'll forgive us. :-) |
|
(but yes - a deprecation message would be a better option) |
|
@geniou I'll try to implement something along those lines. |
|
@nevir what do you think about the current pull request? Are there any open issues from your point of view? I'm not sure if we should release a new version, but maybe its worth putting it into the master to get some feedback? The namespace topic is work in progress, but I'm not sure if its worth waiting, since its not clear when a new rubocop version with this feature will be released. I thin I could live with the Prefixes for now. Maybe we have to put a note into the README, that this will change in the future. |
| expect(cop.offenses.size).to eq(1) | ||
| expect(cop.offenses.map(&:line).sort).to eq([4]) | ||
| expect(cop.messages) | ||
| .to eq(['Use `described_class` for tested class / module']) |
There was a problem hiding this comment.
Hmm, this is an interesting case, I think the error message should contain the expected class/module to help disambiguate for nested describes like this
There was a problem hiding this comment.
Good idea. I'll change it.
|
This PR looks good to me (just the one comment above) Re: release process, I think it's worth getting this out there. There don't appear to be too many users, and - hopefully - most are forgiving of breaking interface changes. It does make our versioning a bit odd, though; since - I think - it's more important to keep versions roughly in sync w/ rubocop, rather than doing the whole semver thing |
|
I'm not so sure about the version numbers. For example there is a rubocop version |
|
Yeah, that sucks, and is a maintenance headache :-/ (which we see now, even) How about we just do the semver approach and fully decouple? 1.0(pre) now, (0.x being special cased as sometimes breaking changes is never handled
|
|
@nevir you cosed it without mering. Was this on purpose? |
|
I like the idea of |
|
Nope, mistake! |
|
Yeah, let's go w/ 1.0pre |
|
Actually, might as well do the rc# approach, juuuust in case. |
No description provided.