-
Notifications
You must be signed in to change notification settings - Fork 9
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[rubocop] Step 5 #214
[rubocop] Step 5 #214
Conversation
Because enabling it, even if it's recommended, require very careful tests and review to avoid accidental runtime crashes
Used RubyMine's refactoring feature to extract the big chunk of code building the entries into its private method, to reduce the self.run(…)'s complexity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM – I left a few questions, but nothing blocking
lib/fastlane/plugin/wpmreleasetoolkit/helper/promo_screenshots_helper.rb
Show resolved
Hide resolved
# Enabling the 'frozen_string_literal: true' comment is nice but should be done with extra care | ||
# because enabling it on code that accidentally mutates string literals will crash at runtime | ||
# (e.g. `name = 'Hello'` then later `name << ' world'` will crash if the comment is enabled) | ||
# So we might enable it at some point, be we will need very careful review that this doesn't break everything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
lib/fastlane/plugin/wpmreleasetoolkit/helper/an_metadata_update_helper.rb
Show resolved
Hide resolved
lib/fastlane/plugin/wpmreleasetoolkit/actions/common/promo_screenshots_action.rb
Outdated
Show resolved
Hide resolved
Change the setting of the rule from the default (no_comma) to consistent_comma, which better matches our style
Thanks for the review @jkmassel 👍 I should have addressed all your questions and suggestions 🙂 |
Continuing work on #207 and rubocop.
This also includes a small refactor (consisting of the extraction of long portion of code into a separate method, to reduce the code complexity and method length, see f84285d) that might also need careful review – especially to ensure that all the variables used in the original code were all passed to the method and that the extracted code didn't modify any variable that would then later be used after it in the original code, etc.
The corresponding commits are labeled accordingly (
[rubocop] fix … (manual)
and the like) – in case you want to review commit-per-commit.