Add rubocop #20
Add rubocop #20
Conversation
.rubocop_todo.yml
Outdated
| @@ -24,7 +24,7 @@ Metrics/ClassLength: | |||
| Metrics/CyclomaticComplexity: | |||
| Max: 11 | |||
|
|
|||
| # Offense count: 154 | |||
| # Offense count: 151 | |||
| # Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns. | |||
| # URISchemes: http, https | |||
| Metrics/LineLength: | |||
There was a problem hiding this comment.
Let's set max line length to 100, which is consistent across all Percy codebases. Offending lines should probably be fixed.
There was a problem hiding this comment.
Err, I mean to set this in rubocop config.
|
Ready for review. I'm going to have another quick scan too. |
| @@ -46,7 +44,8 @@ def snapshot(page, options = {}) | |||
| return | |||
| end | |||
|
|
|||
| snapshot['data']['relationships']['missing-resources']['data'].each do |missing_resource| | |||
| snapshot['data']['relationships']['missing-resources']['data'] | |||
There was a problem hiding this comment.
Instead of line-breaking this on the .each, will you change this to set this thing to a variable and then just iterate over it on the next line?
| @@ -15,7 +14,7 @@ class NativeLoader < BaseLoader | |||
| LOCAL_HOSTNAMES = [ | |||
| 'localhost', | |||
| '127.0.0.1', | |||
| '0.0.0.0', | |||
There was a problem hiding this comment.
I’d like to keep (and maybe enforce, if possible) trailing commas on lists.
| @@ -123,7 +125,7 @@ | |||
| end | |||
| describe '#finalize_current_build' do | |||
| it 'finalizes the current build' do | |||
| build_data = {'data' => {'id' => 123}} | |||
| build_data = { 'data' => { 'id' => 123 } } | |||
There was a problem hiding this comment.
I'd like to enforce the opposite of this, also. ie. without inner space padding inside hashes.
| } | ||
| stub_request(:post, builds_api_url) | ||
| .to_return(status: 201, body: mock_response.to_json) | ||
| capybara_client.initialize_build | ||
|
|
||
| loader = capybara_client.initialize_loader | ||
| expect(capybara_client.send(:_upload_missing_build_resources, loader.build_resources)).to eq(0) | ||
| expect(capybara_client.send(:_upload_missing_build_resources, loader.build_resources)) | ||
| .to eq(0) |
There was a problem hiding this comment.
Same here, instead of line-breaking on the .to, can you rewrite this to use a variable? Same number of lines, more readable.
result = capybara_client.send(:_upload_missing_build_resources, loader.build_resources)
expect(result).to eq(0)
| @@ -18,50 +18,50 @@ | |||
| loader = described_class.new(page: page) | |||
| resource_urls = loader.snapshot_resources.collect(&:resource_url) | |||
| expect(resource_urls).to match_array([ | |||
| "/test-css.html", | |||
There was a problem hiding this comment.
I'd like to enforce the opposite of this too, to keep it as is. One of my most strong style preferences is to try to never enforce whitespace indentation that has to match lines above, because then if the line length above changes (which it does all the time), I'd have to micromanage the whitespace of the following lines which is just plain-out overhead (the same reason why I don't align = value lines and always just left-align them). Especially in this case, it definitely doesn't help readability to align them. I'd like to enforce 2-space indent indentation for these kinds of blocks.
There was a problem hiding this comment.
@fotinakis What if Rubocop managed this for you?
There was a problem hiding this comment.
Hmm, still feel the same way on this one. I like the auto-formatting, but the same problems still apply and I really don't think it's more readable for the problems it causes—even if you don't have to micromanage the front spacing, it might push the lines over line-length limits so then you'd have to try to micro-manage line-breaks.
| } | ||
| # Stub create build. | ||
| build_stub = stub_request(:post, builds_api_url) | ||
| .to_return(status: 201, body: mock_response.to_json) | ||
| .to_return(status: 201, body: mock_response.to_json) |
There was a problem hiding this comment.
Same here as other comment (but maybe a different rubocop config), I'd like to enforce a 2-space (or 4-space, arguably) indent for continued line-break lines, and not align them to the line above.
This adds rubocop, and a todo file for rules we can clean up as we move forward.
The second commit is the changes rubocop has made automatically.