Add rubocop and resolve lint errors

[tegandbiscuits]

Context

• Fixes Setup linting #643

Summary of Changes

Sets up rubocop with rubocop-rspec and rubocop-rails, and resolves warnings/errors that it spits out. Mostly sticking to the default settings for the rules.

Given that these changes were largely cosmetic and to prevent confusion, and that the test suite seems pretty extensive, I don't believe any of these changes will have side effects. For anything that I wasn't sure about fixing, I just added a rubocop:disable comment.

I also added it to the Travis CI script so future pull requests need to pass lint.

Checklist

• CI Passes
• Deploys to Heroku on test Correctly (Maintainers will handle)
• Added Documentation (Service and Code when required)

[tegandbiscuits]

With this setting if an update to rubocop adds a new rule it'll automatically be enabled, which could cause the test suite to fail linting.

I could remove this setting so it just logs that a rule has been added, but in my experience new rules don't usually cause issues and not automatically enabling causes more work to have to manually enable them.

[DeeDeeG]

This is good as-is. I think your reasoning sounds good; I can agree with that approach.

[tegandbiscuits]

This is the rule for checking that strings use single quotes or double quotes. Right now there's a mixture, so this should probably be enabled for consistency. I turned it off so this pull request touches less things, but I could enable it now or later if desired.

The default is single quotes unless it'll make a difference, but I could put it for double.

[DeeDeeG]

The default sounds good to me.

I think double-quote usage should be reserved for when it means something, so as to draw attention to whatever advanced/interpolation stuff is going on in our strings.

[DeeDeeG]

This Pull Request is already rather large, so I personally think it would be okay to include the quotation mark normalization here as well. If it is committed in a neat way here, that would make it easy to review.

However, maybe saving it for a separate Pull Request is good for this reason: We have "squash pull requests" enabled at this repository. So this entire Pull Request is destined to become one commit on the develop branch. Perhaps we should want the quotation mark normalization done separately so it stays a separate commit on develop branch.

[DeeDeeG]

Again, thanks for the thoughtful approach to enabling this linter. Anticipating some of the pitfalls, however minor, increases my confidence with adding the linter and is making the experience better of adding it to the repository. It's personally my first time using Rubocop, so that's great.

[tegandbiscuits]

For anyone unaware, creating a hash like { foo: 'bar' } and { :foo => 'bar' } is the same (=> is only needed if you don't want the key to be a symbol).

[tegandbiscuits]

I'm not 100% sure if the == was the desired effect for this.

[DeeDeeG]

Hi,

I think this is meant to check if the assignment works (thereby checking that results.first exists and isn't undefined/null or what-have-you), and then use the gio object only if that's the case (only if it has become a copy of results.first).

I don't believe geo is defined anywhere else in the project, so the == likely makes reverse_geocoded_by fail to update the obj (object) passed to it. Sorry for the verbose explanation, but I'm explaining it to myself as I go as well. (Said so before, but I haven't edited the Ruby code much -- I have read much of it though several times, just not 100% understanding as I'm so-so with Ruby).

[DeeDeeG]

Regardless of the intent or correctness, I presume this was copy-pasted and lightly modified from the geocoder gem's README.md:

https://github.com/alexreisner/geocoder/tree/v1.6.4#custom-result-handling

[tegandbiscuits]

This expect didn't seem to run before these changes.

Also if you're open to it, I wouldn't mind adding something like expect(json).not_to be_empty above line 12 in order to prevent false positives if this loop never runs.

[DeeDeeG]

I think adding expect(json).not_to be_empty sounds good. Thanks for noticing this!

Is the # TODO comment still accurate? Or if this expect() is running now, then I suppose the comment can/should be deleted.

[tegandbiscuits]

I changed the wording because of the RSpec/ContextWording rule (this rule is also why I changed some context blocks to describe since it seemed more semantic).

If I'm very off base with the wording I can just disable this rule.

[DeeDeeG]

I appreciate the more semantic wording! I like it when the error messages read like natural language and make sense as sentences.

And so, I was actually meaning to go back and review these and just make sure the new wordings still captured the intent or explain what the test is for in an accurate way. Wanted to help ensure that could happen. I'll see how involved this all gets... Could be some work, but it seems worth it to get it right now for benefit of use later on down the line.

[DeeDeeG]

Thanks for all the work, and the thoughtful comments. I'm finding those helpful.

cc @tkwidmer and @mi-wood I have been copying you both on a lot of things lately, but you are both more expert at Ruby than I am. Your review(s) for this PR would be appreciated.

@tegandbiscuits I am still committed to reviewing this if I don't hear back from either of them. I'm not sure what their schedules look like right now.

[DeeDeeG]

I'm going to call this accepted, for hacktoberfest purposes. I should get a chance some time to really review the changes and possibly revert some small things and/or tweak the settings. But this is passing tests, and it mostly looks good.

Thank you!

[tegandbiscuits]

@DeeDeeG I'll revert back the = tonight since that could cause problems.

Thanks!

[tegandbiscuits]

Good catch about the typo!

results is defined by the parameter to the block, and it looks like the conditional is checking that it's not empty (which it presumably could be if no location was found).

I tried it out with

results = []
puts 'hello' if x = results.first

results = ['something']
puts 'hello' if x = results.first

[mi-wood]

This generally lgtm. Thanks! I think we had houndci for linting at some point and it was causing a bit too much noise, but never followed up to readd something else.

Only thing I'd add, is that we should loop back to the docs so that people know they can autofix linting and don't spend time trying to manually fix all the coding style.

[DeeDeeG]

Only thing I'd add, is that we should loop back to the docs so that people know they can autofix linting and don't spend time trying to manually fix all the coding style.

As long as there's an easy command to run, I'd be happy to cook up some wording for CONTRIBUTING.md about this.

Seems like it's rubocop --auto-correct (per a documentation page I found: https://docs.rubocop.org/rubocop/0.93/usage/auto_correct.html#safe-auto-correct).

[tegandbiscuits]

@DeeDeeG sorry it took a couple days to reply. I added a bit of docs to CONTRIBUTING.md for linting. Let me know if you want me to change it (or you could change it yourself if you'd rather).

[DeeDeeG]

👍 thanks for adding documentation. Awesome. ❤️

[DeeDeeG]

@tegandbiscuits no worries on response time. As you can see, we all have stuff going on and I also take some time to respond...

I want to make sure I finish up the review process here and get this merged before any other Pull Requests land. The intent is not to make you have to lint other people's work. Sorry about that.

Here's what I will do: I will put up a Heroku instance (hosted instance of the web app) and do a manual walk-through of all the features. If everything works, I think we can go ahead and merge this.

Thanks for all your work. If you're looking to complete your five pull requests for Hacktoberfest still (and I suppose even if you're not), I would welcome the quotation mark normalization as a second pull request. That one might be simpler, but a lot of work had been put in already on this main pull request, and I think that counts as at least two typical pull requests' worth of work. Regardless of points and t-shirts and whatnot (regarding the Hacktoberfest event), this PR took a lot of time and work and I appreciate it.

[DeeDeeG]

Everything works in manual testing.

Planning to merge this tomorrow.

(Thanks again!)

[DeeDeeG]

Hi again,

There's some very small changes I would like to suggest relating to the merge conflicts and how they were resolved.

Essentially, I would revert any changes in Gemfile.lock and yarn.lock from the "merge master"/"merge develop" commits onward.

Tip: how to fix on the command-line (click to expand)

• You can do this on the command-line with git restore --source=706a669 Gemfile.lock yarn.lock
  • 706a669 is the commit in this pull request just before the merges. The above command restores the files to the state from that commit.
• If you have git older than 2.23, the git restore command isn't available.
  • You can do this command instead: git checkout 706a669 -- Gemfile.lock yarn.lock

And I noticed there is a duplicated bit of code in spec/support/rspec.rb:

    recaptcha_response = { 'success' => true }
    stub_request(:post, 'https://www.google.com/recaptcha/api/siteverify')
      .to_return(status: 200, body: recaptcha_response.to_json,
                 headers: { 'Content-Type' => 'application/json' })

I would:

• Delete this duplicate bit of code
  • The block of code apparently becomes shorter, so the comment # rubocop:disable Metrics/BlockLength isn't needed anymore, and can be removed.
  • The corresponding # rubocop:enable Metrics/BlockLength isn't needed.

If you want to address this yourself, I can leave it for you. I also have a copy of this on my machine with the changes, which I could upload to your branch if you don't mind. Once that's resolved, I'll be happy to merge this. As always, thank you for this contribution.

Best Regards,

- DeeDeeG

[tegandbiscuits]

@DeeDeeG it’d probably be faster if you could do it. Though I could do it this weekend if it’d be longer than that.

[DeeDeeG]

A funny thing happened... Rubocop has been around since roughly 2013, judging by their CHANGELOG.md... but they made a stable 1.0.0 release just yesterday! What timing!

https://github.com/rubocop-hq/rubocop/blob/master/CHANGELOG.md

I suppose it would be a good idea to upgrade to Rubocop 1.0 soon. For now, this pull request is fine using Rubocop 0.92, and will be merged as-is. There are some other pending pull requests that would be good to merge as soon as this one is merged, so Rubocop 1.0 can wait.