Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Rubocop - the sequel #1441

Merged
merged 13 commits into from Jun 8, 2022
Merged

Enable Rubocop - the sequel #1441

merged 13 commits into from Jun 8, 2022

Conversation

timrogers
Copy link
Contributor

@timrogers timrogers commented Jun 7, 2022

This PR is a follow up to #1432, again trying to enable Rubocop in CI and autocorrect what we can, whilst keeping the gem actually working 馃槄 鉂わ笍

I decided to start again with a fresh PR because the original PR had a very ugly history - lots of embarrassing fiddling around getting the right TargetRubyVersion for Rubocop! - and I didn't want to destroy it as there were some useful comments in there.

It's worth noting that, to support linting compatible with Ruby 2.3 (which is the current version we actually support, despite what the gemspec claims!), we have to downgrade Rubocop to an earlier version.

@timrogers
Copy link
Contributor Author

Theoretically, Rubocop autocorrection shouldn't lead to any substantive changes in the meaning of your code, but it can make mistakes from time to time. I have a high level of confidence though that we're safe: the tests pass, and I have manually read through the changes to check for anything evident (although this is obviously challenging given the length of the diff).

Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

鉂わ笍

@timrogers timrogers force-pushed the enable-rubocop-redux branch 2 times, most recently from a735bc7 to b304823 Compare June 8, 2022 11:05
ERB in Ruby 2.3 doesn't seem to be frozen string literal
compatible - at least under certain cases. I observed a number
of test failures in GitHub Actions when working with templates,
generating an error like this:

```
(erb):1:in `concat': can't modify frozen String (RuntimeError)
```

You can see an example [here][1].

It seems prudent just to ignore this since (a) people are unlikely
to use Ruby 2.3, (b) we are set to drop support for it and (c)
it's even more unlikely that people use Ruby 2.3 *and* frozen
string literals.

[1]: https://github.com/octokit/octokit.rb/runs/6792298625?check_suite_focus=true
@nickfloyd nickfloyd added Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR Type: Feature New feature or request and removed housekeeping labels Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants