Document more cops with readups #82
Conversation
| describe "#{cop.name}" do | ||
| it "has content" do | ||
| resolver = ContentResolver.new(cop.name) | ||
| if !whitelist.include?(cop.name) |
There was a problem hiding this comment.
We should probably filter the cops with the whitelist before the each above. As-is, you're running a spec that says something like RuboCop::Cop::Lint::AssignmentInCondition has content, but is a no-op and does not actually assert that spec has content (which is of course intentional).
There was a problem hiding this comment.
Good point! Updated
| whitelist = File.read("./spec/support/docs_whitelist.txt").lines.map(&:chomp) | ||
|
|
||
| it "has cops" do | ||
| expect(cops.count).to eq 310 |
There was a problem hiding this comment.
Minor: This might get annoying to assert when we upgrade rubocop. Do we really care about the exact number? If new ones are added & missing content, other specs will fail and we'll need to add them to the whitelist or write content, right?
There was a problem hiding this comment.
That's true. I added this because I wanted to make sure RuboCop::Cop::Cop.all didn't return empty array, because if it did than the other tests wouldn't run at all. There's no reason to think that will happen, it was just a sanity check, mostly. WDYT about keeping it, but making it less specific? Happy to just get rid of it as well
There was a problem hiding this comment.
I wondered if that was why you did it. I think if you wanted that sanity check, > 0 would be sufficient. But since you're asking Rubocop directly for the cop classes, I also wouldn't expect this to randomly break & return 0: if Rubocop changes the interface, I would expect it to blow up much more severely than that.
lib/cc/engine/content_resolver.rb
Outdated
| end | ||
|
|
||
| def content | ||
| File.exist?(content_path) && File.read(content_path) |
There was a problem hiding this comment.
💄 probably want to memoize this as well.
|
Small notes/questions, but LGTM overall. |
| @@ -0,0 +1,174 @@ | |||
| RuboCop::Cop::Lint::AssignmentInCondition | |||
There was a problem hiding this comment.
Would "undocumented_cops" be a better name for this file? I didn't really get its purpose the first time I glanced at this PR.
We are missing 175 cops now... Adding them all will be tricky.
maxjacobson
changed the title
3 participants
These make CC more useful. I walked @larkinscott through how to add them. We'll help provide examples as needed.
The missing cops (175 of them) are in a whitelist file to keep the build green. I propose un-whitelisting the "Lint" cops for starters.
Feel free to offer thoughts on this problem/approach.