Add rubocop and resolve lint errors

[tegandbiscuits]

Context

• Fixes #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)

[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

[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]

@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]

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.