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

[Ruby 3.0] Bump rubocop version #18564

Merged
merged 16 commits into from
Apr 22, 2021
Merged

Conversation

ainame
Copy link
Contributor

@ainame ainame commented Apr 16, 2021

This PR has about 600 LoC changes in automatically generated files (.rubocop_todo.yml and Gemfile.lock) so the actual changes are small. Don't be scared馃槆

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

Prior to this PR, as per #17931, I've been working on Ruby 3.0 support tasks. This PR bumps the long-standing very old rubocop version (0.49.1) to the latest (1.12.1) and hopefully enables us to keep it up-to-date. rubocop is the most popular linter tool in Ruby and essential for fastlane to maintain the codebase consistent. However, the version we fix doesn't support Ruby 3.0 due to the version requirement spec in Rubocop. This gem https://github.com/whitequark/parser to parse Ruby code, whose rubocop depends on, don't support Ruby 3.0 until 3.0.0.0. Meaning we won't be able to run the linter against Ruby 3.0 when it gets supported in fastlane until bumping the rubocop version馃挜 This is how gemspec in rubocop v0.49.1 looks like 馃槆
https://github.com/rubocop/rubocop/blob/1c2245df09cf0a1659b83acf0abb473e9b927a1a/rubocop.gemspec#L29

That's why this PR is here馃檪

Description

The thing is that we need to catch up with many changes in rubocop. rubocop is adding new rules(cops) and improving existing ones continuously so that we can see a lot of warnings when running the latest version against fastlane. I'm going to introduce .rubocop_todo.yml to address that problem.

.rubocop_todo.yml is an automatically generated file that is meant to disable overwhelming rubocop's warnings for existing project for now in order to fix them gradually. This way we don't really need to fix such issues pointed by rubocop in this bumping PR and we can improve them later in future PRs.

https://docs.rubocop.org/rubocop/configuration.html#automatically-generated-configuration

Besides that what I did in this PR are -

  • Fixed .rubocop.yml to be able to run the latest rubocop (there are some cops that aren't supported any more)
  • Migrated some cops's department (namespace) as those are renamed to the different department; i.e. Style/VariableName -> Naming/VariableName
  • Added rubocop-performance to gemspec as it spun off from rubocop gem
  • Fixed custom cops implemented in fastlane to catch up on the latest API in rubocop
  • Fixed some issues pointed that are missed by rubocop even though we enabled the rules 6a0598f
  • Fixed how the plugin's .rubocop.yml is generated to make it work
  • Update one line of code to stop rubocop's crash 14ea64a

I will add more information about changes with inline comments.


Next, I will submit another PR that applies auto-correction to fastlane in one go based on the latest rubocop so that we can fix many issues in TODO quickly. And then anyone will be able to contribute by picking issues in .rubocop_todo.yml.

Note that rubocop 1.13.0 was released on 20th April but it dropped Ruby 2.4 support so we can't upgrade to it for now but once we support Ruby 3.0 we can keep rubocop up-to-date again馃檪

Testing Steps

See if CI passes linter checks

@ainame ainame force-pushed the bump-rubocop-version branch 2 times, most recently from eb353dd to c63c67b Compare April 21, 2021 06:30
- ./rubocop/fork_usage.rb
- ./rubocop/missing_keys_on_shared_area.rb
- ./rubocop/is_string_usage.rb

AllCops:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is moved to the top to make it prominent.

Comment on lines +13 to +18
Include:
- '**/*.rb'
- '**/*file'
- '**/*.gemspec'
- '*/lib/assets/*Template'
- '*/lib/assets/*TemplateAndroid'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now required to declare what is included explicitly.

Comment on lines +1 to +7
# This configuration was generated by
# `rubocop --auto-gen-config --exclude-limit 0`
# on 2021-04-21 06:50:09 UTC using RuboCop version 1.12.1.
# The point is for the user to remove these configuration records
# one by one as the offenses are removed from the code base.
# Note that changes in the inspected code, or installation of new
# versions of RuboCop, may require this file to be generated again.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is generated automatically. So no need to review unless you are interested in seeing what rules are added馃槃

Comment on lines -201 to -202
Metrics/PerceivedComplexity:
Max: 18
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that logic was changed and some files don't meet this threshold any more. So I removed it here and rubocop put "Max: 26" in the TODO file.

# coding: utf-8

lib = File.expand_path("../lib", __FILE__)
lib = File.expand_path("lib", __dir__)
Copy link
Contributor Author

@ainame ainame Apr 21, 2021

Choose a reason for hiding this comment

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

Changes in this file and templates are pointed by rubocop. Unlike others, this is needed to be fixed now otherwise issues will be passed to plugins newly generated馃槆

Copy link
Collaborator

Choose a reason for hiding this comment

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

This will need an update of all plugins?

Copy link
Contributor Author

@ainame ainame Apr 22, 2021

Choose a reason for hiding this comment

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

No. This doesn't affect existing plugins/gems. This is just style fix. If plugin authors think they want to follow it, they can just do it by themselves.

@@ -3,14 +3,14 @@ module Languages
# These are all the languages which are available to use to upload app metadata and screenshots

# The old format which was used until August 2015 (good old times)
ALL_LANGUAGES_LEGACY = ["da-DK", "de-DE", "el-GR", "en-AU", "en-CA", "en-GB", "en-US", "es-ES", "es-MX", "fi-FI", "fr-CA", "fr-FR", "id-ID", "it-IT", "ja-JP", "ko-KR", "ms-MY", "nl-NL", "no-NO", "pt-BR", "pt-PT", "ru-RU", "sv-SE", "th-TH", "tr-TR", "vi-VI", "cmn-Hans", "cmn-Hant"]
ALL_LANGUAGES_LEGACY = %w[da-DK de-DE el-GR en-AU en-CA en-GB en-US es-ES es-MX fi-FI fr-CA fr-FR id-ID it-IT ja-JP ko-KR ms-MY nl-NL no-NO pt-BR pt-PT ru-RU sv-SE th-TH tr-TR vi-VI cmn-Hans cmn-Hant]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This exceeded the limit but for some reasons, it wasn't detected.

# The %w might be confusing for new users
Style/WordArray:
  MinSize: 19

@@ -33,7 +33,7 @@ def on_if(node)
def on_send(node)
return unless bad_fork(node)
return if self.good_nodes.include?(node)
add_offense(node, :expression, MSG)
add_offense(node, location: :expression, message: MSG)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add_offsense in rubocop no longer support positional arguments and instead, we need to use keyword arguments.

@@ -2,6 +2,6 @@ module Deliver
module Languages
# 2020-08-24 - Available locales are not available as an endpoint in App Store Connect
# Update with Spaceship::Tunes.client.available_languages.sort (as long as endpoint is avilable)
ALL_LANGUAGES = ["ar-SA", "ca", "cs", "da", "de-DE", "el", "en-AU", "en-CA", "en-GB", "en-US", "es-ES", "es-MX", "fi", "fr-CA", "fr-FR", "he", "hi", "hr", "hu", "id", "it", "ja", "ko", "ms", "nl-NL", "no", "pl", "pt-BR", "pt-PT", "ro", "ru", "sk", "sv", "th", "tr", "uk", "vi", "zh-Hans", "zh-Hant"]
ALL_LANGUAGES = %w[ar-SA ca cs da de-DE el en-AU en-CA en-GB en-US es-ES es-MX fi fr-CA fr-FR he hi hr hu id it ja ko ms nl-NL no pl pt-BR pt-PT ro ru sk sv th tr uk vi zh-Hans zh-Hant]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This exceeded the limit but for some reasons, it wasn't detected.

# The %w might be confusing for new users
Style/WordArray:
  MinSize: 19

@@ -23,7 +23,7 @@ def utf8_locale?
def take_off
before_import_time = Time.now

if !ENV["FASTLANE_DISABLE_ANIMATION"]
if ENV["FASTLANE_DISABLE_ANIMATION"].nil?
Copy link
Contributor Author

@ainame ainame Apr 21, 2021

Choose a reason for hiding this comment

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

This stops a crash in rubocop. I would report it to them but in the meantime, we can keep it running with this easy fix.

https://gist.github.com/ainame/b90ba9fc159a0ff2ab8cb417ac9586c0

Copy link
Collaborator

Choose a reason for hiding this comment

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

How weird

@ainame ainame marked this pull request as ready for review April 21, 2021 07:15
@ainame ainame self-assigned this Apr 21, 2021
Copy link
Collaborator

@minuscorp minuscorp left a comment

Choose a reason for hiding this comment

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

What a work here! I'm glad that you didn't take all cops by yourself, you did more than enough! 鉂わ笍馃殌

Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

Oh my goodness... I can鈥檛 thank you enough for this PR 馃檹 This was very much needed and you killed it. This is going to great 鉂わ笍

@joshdholtz joshdholtz merged commit ec629c1 into fastlane:master Apr 22, 2021
@ainame ainame deleted the bump-rubocop-version branch April 22, 2021 06:21
@fastlane-bot
Copy link

Hey @ainame 馃憢

Thank you for your contribution to fastlane and congrats on getting this pull request merged 馃帀
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a week, and your PR will be included in the next one.

Please let us know if this change requires an immediate release by adding a comment here 馃憤
We'll notify you once we shipped a new release with your changes 馃殌

@fastlane-bot
Copy link

Congratulations! 馃帀 This was released as part of fastlane 2.182.0 馃殌

@fastlane fastlane locked and limited conversation to collaborators Jul 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants