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 4 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
3 changes: 3 additions & 0 deletions .rubocop.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ Style/TrailingCommaInArrayLiteral:
Style/FrozenStringLiteralComment:
Enabled: false

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.to_s)
AliSoftware marked this conversation as resolved.
Show resolved Hide resolved
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.to_s, 'strings-*.xml'))
AliSoftware marked this conversation as resolved.
Show resolved Hide resolved
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.to_s)
AliSoftware marked this conversation as resolved.
Show resolved Hide resolved
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.to_s)
AliSoftware marked this conversation as resolved.
Show resolved Hide resolved
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.to_s, 'Localizable_*.strings')) - [tmp_main_file]
AliSoftware marked this conversation as resolved.
Show resolved Hide resolved
extra_strings = []
extra_keys = []
join_files.each do |join_strings|
Expand Down