Rubocop fixes and configuration update #19218

iangmaia · 2023-09-19T14:56:24Z

This PR builds on top of #19149 and configures Rubocop in the project, fixing most of the reported offences.

Some of the offences could be a bit riskier to quickly fix and didn't seem very critical, so I generated a .rubocop_todo.yml to silence them for now.

wpmobilebot · 2023-09-19T15:15:56Z

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr19218-cc10f9d
	Commit	`cc10f9d`
	Direct Download	`jetpack-prototype-build-pr19218-cc10f9d.apk`

Note: Google Login is not supported on these builds.

wpmobilebot · 2023-09-19T15:16:21Z

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr19218-cc10f9d
	Commit	`cc10f9d`
	Direct Download	`wordpress-prototype-build-pr19218-cc10f9d.apk`

Note: Google Login is not supported on these builds.

peril-wordpress-mobile · 2023-10-05T18:26:33Z

	Warnings
⚠️	PR is not assigned to a milestone.
⚠️	PR has more than 300 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

mokagio

Thank you for tackling RuboCop @iangmaia !

Before moving forward with this, I suggest syncing with @spencertransier . He's been working on removing outdated settings, like GHHELPER_REPO, from other projects. I wouldn't be surprised if this one was next in the list. It'd be a shame if you worked in parallel and generated merge conflicts

mokagio · 2023-10-31T02:42:25Z

Gemfile

 ### Gems needed only for generating Promo Screenshots
 group :screenshots, optional: true do
-    gem 'rmagick', '~> 4.1'
+  gem 'rmagick', '~> 4.1'
 end


@iangmaia 😄 🤖 win!

mokagio · 2023-10-31T02:43:28Z

fastlane/Fastfile

@@ -160,7 +162,7 @@ import 'lanes/release_management_in_ci.rb'

 default_platform(:android)

-before_all do |lane|
+before_all do |_lane|


I believe RuboCop prefixed this with _ because it's unused. But if that's the case, we might as well remove it, no?

Suggested change

before_all do |_lane|

before_all do

Yes, this was auto-fixed by RuboCop.
In principle I agree, but at the same time, the signature does help to document that this is a special Fastlane thing 🤔

My two cents are that it's up to us devs to look into the documentation to discover what optional parameters the method we use have. I prefer the code to be leaner, every unused parameter begs the question "why is this here?"

But, not something I feel so strongly about to delay this PR for it.

True, the _ RuboCop requests brings in a bit more obscurity to the fact parameter is there. Decided to go with your suggestion: cc10f9d

mokagio · 2023-10-31T02:48:59Z

fastlane/Fastfile

@@ -48,8 +50,8 @@ fastlane_require 'dotenv'
 USER_ENV_FILE_PATH = File.join(Dir.home, '.wpandroid-env.default')
 Dotenv.load(USER_ENV_FILE_PATH)

-ENV[GHHELPER_REPO = 'wordpress-mobile/WordPress-Android']
-ENV['PROJECT_ROOT_FOLDER'] = File.dirname(File.expand_path(__dir__)) + '/'
+ENV.fetch(GHHELPER_REPO = 'wordpress-mobile/WordPress-Android', nil)


I'm not sure about this change.

I think what the code did (or tried to do) before was setting the GHHELPER_REPO value in the environment. Now it's only reading it, I think. 🤔

By the way, do we even need it in the environment? I thought we removed it a while back. See wordpress-mobile/WordPress-iOS#20072

This went away with the latest rebase, so I believe @spencertransier already worked on some refactoring here.

It seems that the old code and the RuboCop updated code weren't doing much 😅

I believe @spencertransier already worked on some refactoring here.

I thought that, too, but couldn't find the PR. I've seen many of those recently because Spencer has been busy across all our apps and assumed I just got confused.

mokagio · 2023-10-31T02:49:51Z

fastlane/lanes/build.rb

-      unless options[:skip_confirm]
-        UI.user_error!('Aborted by user request') unless UI.confirm('Do you want to continue?')
-      end
+      UI.user_error!('Aborted by user request') if !options[:skip_confirm] && !UI.confirm('Do you want to continue?')


What do you think of using positive logic here and in the other instances?

Suggested change

UI.user_error!('Aborted by user request') if !options[:skip_confirm] && !UI.confirm('Do you want to continue?')

UI.user_error!('Aborted by user request') unless options[:skip_confirm] || UI.confirm('Do you want to continue?')

Ah, nice one. RuboCop changed it to avoid nesting, I believe, but the unless with a positive condition is more readable. Updated here: 407518f

… omission

iangmaia · 2023-10-31T12:54:00Z

Thank you for tackling RuboCop @iangmaia !

Before moving forward with this, I suggest syncing with @spencertransier . He's been working on removing outdated settings, like GHHELPER_REPO, from other projects. I wouldn't be surprised if this one was next in the list. It'd be a shame if you worked in parallel and generated merge conflicts

Good point -- in fact, I already had quite some conflicts on several (including this one) of the open PRs in the apps, for both Dangermattic and RuboCop config. Hopefully we'll merge the PRs soon and it will be easier when all branches are aligned on RuboCop and Ruby 3 setup.

mokagio

Approved with non blocking syntax sugar suggestions

mokagio · 2023-11-01T04:49:55Z

fastlane/Fastfile

@@ -160,7 +162,7 @@ import 'lanes/release_management_in_ci.rb'

 default_platform(:android)

-before_all do |lane|
+before_all do |_lane|


My two cents are that it's up to us devs to look into the documentation to discover what optional parameters the method we use have. I prefer the code to be leaner, every unused parameter begs the question "why is this here?"

But, not something I feel so strongly about to delay this PR for it.

mokagio · 2023-11-01T04:50:21Z

fastlane/lanes/build.rb

@@ -158,7 +154,7 @@
      rescue FastlaneCore::Interface::FastlaneError => e
        # Sometimes the upload fails randomly with a "Google Api Error: Invalid request - This Edit has been deleted.".
        # It seems one reason might be a race condition when we do multiple edits at the exact same time (WP beta, JP beta). Retrying usually fixes it
-        if e.message.start_with?('Google Api Error') && (retry_count -= 1) > 0
+        if e.message.start_with?('Google Api Error') && (retry_count -= 1).positive?


mokagio · 2023-11-01T04:53:09Z

fastlane/lanes/release.rb

-      end rescue puts "Cannot delete file #{file}"
+      File.delete(file) if Integer(File.basename(file, '.*')) < Integer(options[:build])
+    rescue StandardError
+      puts "Cannot delete file #{file}"


Out of scope, but what do you think of relying on Fastlane's UI management?

Suggested change

puts "Cannot delete file #{file}"

UI. error("Could not delete file #{file}.")

Good idea! Done here: cc10f9d

fastlane/lanes/release_management_in_ci.rb

`editorial_branch` should already be a `String` Co-authored-by: Gio Lodi <gio.lodi@automattic.com>

wpmobilebot · 2023-11-01T14:57:16Z

	3 Warnings
⚠️	Gemfile was changed without updating its corresponding Gemfile.lock. Please run `bundle install` or `bundle update <updated_gem>`.
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️	PR is not assigned to a milestone.

Generated by 🚫 Danger

iangmaia added the Do Not Merge In PRs with this label, our automation will fail a require check, preventing accidental merging label Sep 19, 2023

iangmaia changed the title ~~Configure Rubocop~~ Rubocop fixes and configuration improvements Sep 20, 2023

iangmaia force-pushed the fix/rubocop branch from f9c0cf7 to 0f28056 Compare September 21, 2023 09:03

iangmaia changed the title ~~Rubocop fixes and configuration improvements~~ Rubocop fixes and configuration update Sep 21, 2023

iangmaia force-pushed the add/dangermattic-setup branch from a195b05 to bae6cdb Compare October 5, 2023 18:22

iangmaia force-pushed the fix/rubocop branch from 0f28056 to 099fea1 Compare October 5, 2023 18:26

iangmaia force-pushed the add/dangermattic-setup branch from bae6cdb to 7f070cc Compare October 13, 2023 07:51

iangmaia force-pushed the fix/rubocop branch from 099fea1 to 7f83f4f Compare October 13, 2023 08:02

iangmaia force-pushed the add/dangermattic-setup branch from 7f070cc to 666fcad Compare October 13, 2023 09:13

iangmaia force-pushed the fix/rubocop branch from 7f83f4f to 288fcd5 Compare October 13, 2023 09:14

iangmaia force-pushed the add/dangermattic-setup branch from 666fcad to 945e708 Compare October 23, 2023 12:37

iangmaia force-pushed the fix/rubocop branch 2 times, most recently from a10a127 to 946ab0b Compare October 23, 2023 16:45

iangmaia force-pushed the add/dangermattic-setup branch from fe688cb to c5d00d5 Compare October 24, 2023 10:08

iangmaia force-pushed the fix/rubocop branch 2 times, most recently from 1675bc3 to 2ba7613 Compare October 25, 2023 17:38

iangmaia force-pushed the add/dangermattic-setup branch from f123b73 to 37809c8 Compare October 26, 2023 17:17

iangmaia force-pushed the fix/rubocop branch from e7440ba to d1aeab6 Compare October 26, 2023 17:19

iangmaia mentioned this pull request Oct 26, 2023

Dangermattic Setup #19149

Merged

iangmaia added Tooling and removed Do Not Merge In PRs with this label, our automation will fail a require check, preventing accidental merging labels Oct 26, 2023

iangmaia marked this pull request as ready for review October 26, 2023 17:42

iangmaia requested review from mokagio and removed request for mokagio October 30, 2023 10:06

mokagio reviewed Oct 31, 2023

View reviewed changes

iangmaia force-pushed the add/dangermattic-setup branch from 57fe61a to ec62a0b Compare October 31, 2023 09:43

iangmaia force-pushed the fix/rubocop branch from bf7b7f2 to 344bb40 Compare October 31, 2023 09:49

Base automatically changed from add/dangermattic-setup to trunk October 31, 2023 10:24

iangmaia added 7 commits October 31, 2023 12:11

Run rubocop -a

b271cf4

Run rubocop -A

d3b83d0

Fix rubocop offenses manually

d3f6bdf

Configure rubocop to silence remaining offenses

282715c

Fix Rubocop offenses after rebase

1b03d61

Update Rubocop config to not allow Ruby 3.1 hash and arguments values…

7458f06

… omission

Fix Rubocop offense after rebase

059a961

iangmaia force-pushed the fix/rubocop branch from 5f9ca29 to 059a961 Compare October 31, 2023 11:11

Change to a positive condition check so that it is more readable

407518f

mokagio approved these changes Nov 1, 2023

View reviewed changes

iangmaia and others added 2 commits November 1, 2023 15:44

Update fastlane/lanes/release_management_in_ci.rb

219a63e

`editorial_branch` should already be a `String` Co-authored-by: Gio Lodi <gio.lodi@automattic.com>

Implement PR suggestions

cc10f9d

iangmaia enabled auto-merge November 1, 2023 14:54

iangmaia merged commit b26a9a4 into trunk Nov 1, 2023
20 checks passed

iangmaia deleted the fix/rubocop branch November 1, 2023 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rubocop fixes and configuration update #19218

Rubocop fixes and configuration update #19218

iangmaia commented Sep 19, 2023 •

edited

Loading

wpmobilebot commented Sep 19, 2023 •

edited

Loading

wpmobilebot commented Sep 19, 2023 •

edited

Loading

peril-wordpress-mobile bot commented Oct 5, 2023 •

edited

Loading

mokagio left a comment

mokagio Oct 31, 2023

mokagio Oct 31, 2023

iangmaia Oct 31, 2023

mokagio Nov 1, 2023

iangmaia Nov 1, 2023

mokagio Oct 31, 2023

iangmaia Oct 31, 2023

mokagio Nov 1, 2023

mokagio Oct 31, 2023

iangmaia Oct 31, 2023 •

edited

Loading

iangmaia commented Oct 31, 2023

mokagio left a comment

mokagio Nov 1, 2023

mokagio Nov 1, 2023

mokagio Nov 1, 2023

iangmaia Nov 1, 2023

wpmobilebot commented Nov 1, 2023

	UI.user_error!('Aborted by user request') if !options[:skip_confirm] && !UI.confirm('Do you want to continue?')
	UI.user_error!('Aborted by user request') unless options[:skip_confirm] \|\| UI.confirm('Do you want to continue?')

	puts "Cannot delete file #{file}"
	UI. error("Could not delete file #{file}.")

Rubocop fixes and configuration update #19218

Rubocop fixes and configuration update #19218

Conversation

iangmaia commented Sep 19, 2023 • edited Loading

wpmobilebot commented Sep 19, 2023 • edited Loading

wpmobilebot commented Sep 19, 2023 • edited Loading

peril-wordpress-mobile bot commented Oct 5, 2023 • edited Loading

mokagio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iangmaia Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

iangmaia commented Oct 31, 2023

mokagio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpmobilebot commented Nov 1, 2023

iangmaia commented Sep 19, 2023 •

edited

Loading

wpmobilebot commented Sep 19, 2023 •

edited

Loading

wpmobilebot commented Sep 19, 2023 •

edited

Loading

peril-wordpress-mobile bot commented Oct 5, 2023 •

edited

Loading

iangmaia Oct 31, 2023 •

edited

Loading