Adding a Linter

[alexreisner]

Should we add a linter to Geocoder? (Raised by @randoum in #1494.)

[alexreisner]

With regard to Rubocop (and linters generally), in theory, they're great. However, Rubocop is going to find thousands of tiny offenses, and it's going to be tempting to replace every instance of unnecessary double quotes with single quotes, for example. While these changes do improve the code a tiny bit, they also pollute the Git history. IMO, in the case of Geocoder, the tradeoff isn't worth it. We need git blame to be as quick and useful as possible when tracking down bugs. (See also the reasons Rails won't merge cosmetic changes.)

That being said, it would be great to force all future PRs through a linter so we stop adding unnecessary double quotes (and lots of other sub-optimal code) to the repo. I'm certainly open to that.

[randoum]

Those points make perfect sense. And Godfrey's message you linked too.

Now I don't believe it would be possible to instruct Rubocop to work only on the modified lines of a PR. Or do you know a way? Rubocop would search for offenses in the whole project. At best, it might be possible to make it run only on the modified files, but even though it will make lots of noise and confuse contributors.

It's all or nothing :) Take it or leave it! Ahahah, just kidding. But yeah, I understand that it would be a mess to add a linter now.

[alexreisner]

I don't know a way, unfortunately. I was hoping you did. :)

I'm going to close this issue for now, but if anyone can suggest a helpful way of adding a linter, I'm open to the idea.

[randoum]

As an alternative solution, we could ignore the commit from git blame: https://akrabat.com/ignoring-revisions-with-git-blame/ I could fork and test that so we can see if it works as expected.

[alexreisner]

Very interesting. I had no idea that feature existed. Yes, why don't you go ahead and try that?

[randoum]

After few tries, I confirm that there is no way to ignore commit globally, only locally. Bummer.

[alexreisner]

@randoum Darn, that seemed promising. Well, thanks for looking into it.

[sandstrom]

While I agree with the idea of generally not merging cosmetic PRs, I don't think that necessarily needs to block any introduction of a linter.

Any project will need to make larger code base changes now and then. For example, when updating to support a new version of ruby, you'll typically have to edit a bunch of files and make sweeping changes.

Having a few "convert all double quotes" commits here and there isn't a big problem. What chancancode talked about in the issue quoted above, is that they cannot regularly merge cosmetic changes.

If you look at the rails commit history there are many instances of larger refactoring or changes. For example this one that enables a few rubocop checks: https://github.com/rails/rails/pull/32381/files

So in short, I wouldn't shy away from adding a linter and (when doing so) making a bunch of changes to the repo (I'd probably group them in multiple commits though, one per cop, at least the ones covering many files).

---

All of this said, rubocop has a functionality where you can run it on the current code base, and it will create a rubocop_todo.yml file, with all the current "errors". Basically grandfathering in all the past sins. Which is what you discussed above, in https://github.com/alexreisner/geocoder/issues/1496#issuecomment-797449195.

With this mode, you won't have to make any changes to existing code base, but would still help for new code (with some caveates).

[alexreisner]

Thanks @sandstrom for the thoughtful input. That all makes a lot of sense. I didn't know about the rubocop_todo file, and I think that's a great option. I'm going to re-open this for more thought/discussion.