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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve RuboCop offenses #534

Merged
merged 22 commits into from
Sep 13, 2023
Merged

Resolve RuboCop offenses #534

merged 22 commits into from
Sep 13, 2023

Conversation

AlexWayfer
Copy link
Contributor

And add RuboCop GitHub action.

Please note .rubocop_todo.yml config and changes to arguments.

Copy link
Contributor Author

@AlexWayfer AlexWayfer left a comment

Choose a reason for hiding this comment

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

Some important notes.

@@ -87,7 +87,7 @@
* [FFaker::IdentificationESCO](#ffakeridentificationesco)
* [FFaker::IdentificationIN](#ffakeridentificationin)
* [FFaker::IdentificationIT](#ffakeridentificationit)
* [FFaker::IdentificationKr](#ffakeridentificationkr)
* [FFaker::IdentificationKR](#ffakeridentificationkr)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Breaking change. I think it was an error.

autoload_abbreviations.fetch(part) { part.capitalize }
end.join
autoload constant_name, file_name
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

More automatic and short system.

lib/ffaker/avatar.rb Outdated Show resolved Hide resolved
lib/ffaker/book.rb Outdated Show resolved Hide resolved
lib/ffaker/filesystem.rb Outdated Show resolved Hide resolved
lib/ffaker/image.rb Outdated Show resolved Hide resolved
lib/ffaker/image.rb Outdated Show resolved Hide resolved
$warnings << msg if $warnings
return unless Kernel.instance_variable_get(:@ffaker_warnings)

Kernel.instance_variable_set(:@ffaker_warnings, Kernel.instance_variable_get(:@ffaker_warnings) << 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.

Not sure about this, but I don't respect such Kernel#warn overwrite approach at all.

when 5, 7
"#{first_name} #{prefix} #{last_name}"
else
"#{first_name} #{last_name}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strange case. Can we simplify this?

lib/ffaker.rb Outdated Show resolved Hide resolved
lib/ffaker/name_cs.rb Outdated Show resolved Hide resolved
@AlexWayfer AlexWayfer marked this pull request as ready for review September 11, 2023 10:50
Copy link
Member

@marocchino marocchino left a comment

Choose a reason for hiding this comment

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

I checked it all over. LGTM

@marocchino marocchino merged commit 14eed86 into main Sep 13, 2023
4 of 5 checks passed
@marocchino marocchino deleted the resolve_rubocop_offenses branch September 13, 2023 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants