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.

Sign up for GitHub

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

Jump to bottom

[Cleanup/rubocop] 6 #217

Merged
merged 6 commits into from
Feb 25, 2021
Merged

[Cleanup/rubocop] 6 #217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e9cbc69
[rubocop] disable Style/AsciiComments rule
AliSoftware Feb 24, 2021
d096f63
[rubocop] fix Style/Lambda
AliSoftware Feb 24, 2021
83a4d0d
[rubocop] fix Style/UnneededInterpolation
AliSoftware Feb 24, 2021
ee543ca
[ruboco] run safe-auto-correct + auto-gen-config after rebase
AliSoftware Feb 24, 2021
98af7e7
Remove some extra .to_s calls
AliSoftware Feb 25, 2021
70fb5d5
Provide some rationale for custom Style rules
AliSoftware Feb 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .rubocop.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,13 @@ Gemspec/OrderedDependencies:
Lint/UnusedMethodArgument:
Enabled: false

# We have a couple of empty methods especially when we implement an API contract on our Action subclasses
# and those look nicer to us when still written as expended (in separate lines) to suggest us to fill them at some point
# rather than the compact representation of having them one-liner `def foo; end` that is rubocop's default.
Style/EmptyMethod:
EnforcedStyle: expanded

# Trailing commas in array literals helps having smaller diffs when we want to add additional parameters
Copy link
Contributor

@mokagio mokagio Feb 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

👍

Totally out of topic and not really a Ruby friendly thing, but I do love how Elm does trailing commas

[ "this"
, "makes"
, "for"
, "clear"
, "diffs"
]

Style/TrailingCommaInArrayLiteral:
EnforcedStyleForMultiline: consistent_comma

Expand All @@ -29,6 +33,11 @@ Style/TrailingCommaInArrayLiteral:
Style/FrozenStringLiteralComment:
Enabled: false

# Unicode is pretty standard by now, and we want to be able to use em-dash, ellipsis, and unicode characters
# to provide nice Monodraw.app graphs and similar, without being limited only by ascii.
Style/AsciiComments:
Enabled: false
Comment on lines +38 to +39
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy with this but I'm curious about the rationale?

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally, I'd like to have a rationale for every disabled rule or any other variation from the default.

Copy link
Contributor Author

@AliSoftware AliSoftware Feb 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

My rationale is:

  • We already use some Unicode characters in existing comments, like em dash — which I'm a big fan of, I have to admit — and ellipsis… which I use here and there too.
  • It feels reasonable to consider that at some point we might also want to use emojis, as well as some Unicode characters necessary to draw nicer Monodraw graphs — and you know how I love those too 😛.
  • And in the era of Unicode I don't see any reason why we shouldn't allow them. (iow, I don't really see the rationale for the default value of this rule.)

I'll add a comment to explain that rationale in the config file anyway indeed.

TL;DR:
"Jealous Girlfriend" meme variation with em dash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rationale added as comment in 70fb5d5

Copy link
Contributor

Choose a reason for hiding this comment

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

🤣 👍


########## Metrics / Max Lengths Rules

Metrics/LineLength:
Expand Down
32 changes: 1 addition & 31 deletions .rubocop_todo.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# This configuration was generated by
# `rubocop --auto-gen-config`
# on 2021-02-18 21:56:11 +0100 using RuboCop version 0.75.0.
# on 2021-02-24 20:33:55 +0100 using RuboCop version 0.75.0.
# 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
Expand Down Expand Up @@ -218,20 +218,6 @@ Security/Open:
- 'spec/android_merge_translators_strings_spec.rb'
- 'spec/ios_merge_translators_strings_spec.rb'

# Offense count: 12
# Configuration parameters: AllowedChars.
Style/AsciiComments:
Exclude:
- 'lib/fastlane/plugin/wpmreleasetoolkit/actions/configure/configure_setup_action.rb'
- 'lib/fastlane/plugin/wpmreleasetoolkit/actions/configure/configure_update_action.rb'
- 'lib/fastlane/plugin/wpmreleasetoolkit/actions/configure/configure_validate_action.rb'
- 'lib/fastlane/plugin/wpmreleasetoolkit/helper/configure_helper.rb'
- 'lib/fastlane/plugin/wpmreleasetoolkit/helper/git_helper.rb'
- 'lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_git_helper.rb'
- 'lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb'
- 'lib/fastlane/plugin/wpmreleasetoolkit/helper/promo_screenshots_helper.rb'
- 'spec/ios_lint_localizations_spec.rb'

# Offense count: 95
Style/Documentation:
Enabled: false
Expand Down Expand Up @@ -262,14 +248,6 @@ Style/IfInsideElse:
- 'lib/fastlane/plugin/wpmreleasetoolkit/actions/configure/configure_update_action.rb'
- 'lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_version_helper.rb'

# Offense count: 1
# Cop supports --auto-correct.
# Configuration parameters: EnforcedStyle.
# SupportedStyles: line_count_dependent, lambda, literal
Style/Lambda:
Exclude:
- 'lib/fastlane/plugin/wpmreleasetoolkit/actions/common/promo_screenshots_action.rb'

# Offense count: 92
# Cop supports --auto-correct.
# Configuration parameters: IgnoredMethods.
Expand Down Expand Up @@ -480,14 +458,6 @@ Style/UnlessElse:
- 'lib/fastlane/plugin/wpmreleasetoolkit/actions/common/promo_screenshots_action.rb'
- 'lib/fastlane/plugin/wpmreleasetoolkit/actions/configure/configure_download_action.rb'

# Offense count: 5
# Cop supports --auto-correct.
Style/UnneededInterpolation:
Exclude:
- 'lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_merge_translators_strings.rb'
- 'lib/fastlane/plugin/wpmreleasetoolkit/actions/configure/configure_download_action.rb'
- 'lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_merge_translators_strings.rb'

# Offense count: 4
# Cop supports --auto-correct.
Style/ZeroLengthPredicate:
Expand Down
4 changes: 2 additions & 2 deletions lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_merge_translators_strings.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class AndroidMergeTranslatorsStringsAction < Action
def self.run(params)
folder_path = File.expand_path(params[:strings_folder])

subfolders = Dir.entries("#{folder_path}")
subfolders = Dir.entries(folder_path)
subfolders.each do |strings_folder|
merge_folder(File.join(folder_path, strings_folder)) if strings_folder.start_with?('values')
end
Expand All @@ -24,7 +24,7 @@ def self.merge_folder(strings_folder)
tmp_main_file = main_file + '.tmp'
FileUtils.cp(main_file, tmp_main_file)

join_files = Dir.glob(File.join("#{strings_folder}", 'strings-*.xml'))
join_files = Dir.glob(File.join(strings_folder, 'strings-*.xml'))
extra_strings = []
extra_keys = []
join_files.each do |join_strings|
Expand Down
2 changes: 1 addition & 1 deletion lib/fastlane/plugin/wpmreleasetoolkit/actions/common/circleci_trigger_job_action.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def self.available_options
env_name: 'FL_CIRCLECI_JOB_PARAMS',
description: 'Parameters to send to the CircleCI pipeline',
type: Hash,
default_value: nil)
default_value: nil),
]
end

Expand Down
2 changes: 1 addition & 1 deletion lib/fastlane/plugin/wpmreleasetoolkit/actions/common/promo_screenshots_action.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ def self.run(params)

bar = ProgressBar.new(entries.count, :bar, :counter, :eta, :rate)

Parallel.map(entries, finish: ->(_item, _i, _result) {
Parallel.map(entries, finish: lambda { |_item, _i, _result|
bar.increment!
}) do |entry|
device = devices[entry['device']]
Expand Down
2 changes: 1 addition & 1 deletion lib/fastlane/plugin/wpmreleasetoolkit/actions/configure/configure_download_action.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ def self.run(params = {})
UI.message 'Running Configure Download'

# If the `~/.mobile-secrets` repository doesn't exist
unless File.directory?("#{secrets_dir}")
unless File.directory?(secrets_dir)
UI.user_error!("The local secrets store does not exist. Please clone it to #{secrets_dir} before continuing.")
else
update_repository # If the repo already exists, just update it
Expand Down
4 changes: 2 additions & 2 deletions lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_merge_translators_strings.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ class IosMergeTranslatorsStringsAction < Action
def self.run(params)
folder_path = File.expand_path(params[:strings_folder])

subfolders = Dir.entries("#{folder_path}")
subfolders = Dir.entries(folder_path)
subfolders.each do |strings_folder|
merge_folder(File.join(folder_path, strings_folder)) if strings_folder.ends_with?('.lproj')
end
Expand All @@ -21,7 +21,7 @@ def self.merge_folder(strings_folder)

UI.message("Merging in: #{strings_folder}")

join_files = Dir.glob(File.join("#{strings_folder}", 'Localizable_*.strings')) - [tmp_main_file]
join_files = Dir.glob(File.join(strings_folder, 'Localizable_*.strings')) - [tmp_main_file]
extra_strings = []
extra_keys = []
join_files.each do |join_strings|
Expand Down