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

RuboCop: Stage IV #171

Merged
merged 1 commit into from
Nov 21, 2024
Merged

RuboCop: Stage IV #171

merged 1 commit into from
Nov 21, 2024

Conversation

tagliala
Copy link
Contributor

@tagliala tagliala commented Oct 10, 2024 •
edited
Loading

Requires #170

I've tried to only fix objective or almost-objective offenses, if you don't like some of the fixes let me know.

There are very few changes to production code

This PR fixes:

  • Layout/DotPosition
  • Lint/AmbiguousOperator
  • Lint/LiteralInInterpolation
  • RSpec/BeEmpty
  • RSpec/ContextMethod
  • RSpec/HookArgument
  • RSpec/MatchArray
  • RSpec/NotToNot
  • Style/EachWithObject
  • Style/ExpandPathArguments
  • Style/HashSyntax
  • Style/NestedParenthesizedCalls
  • Style/RedundantBegin
  • Style/RedundantConstantBase
  • Style/RedundantReturn
  • Style/RegexpLiteral

Some manual fixes:

  • Lint/RedundantSplatExpansion and subsequent unsafe
    manual fix of Style/HashConversion
  • Style/NonNilCheck and subsequent switch from
    select !nil to reject nil

@tagliala tagliala force-pushed the chore/rubocop-stage-iv branch from 33b654b to 9d62078 Compare October 10, 2024 07:15
tagliala
tagliala commented Oct 10, 2024
View reviewed changes
lib/inline_svg.rb
@@ -66,7 +65,7 @@ def add_custom_transformation(options)
raise InlineSvg::Configuration::Invalid.new("#{options.fetch(:transform)} should implement the .create_with_value and #transform methods")
end

@custom_transformations.merge!(Hash[*[options.fetch(:attribute, :no_attribute), options]])
@custom_transformations.merge!(options.fetch(:attribute, :no_attribute) => options)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Manual fix 1: looks safe to me

tagliala
tagliala commented Oct 10, 2024
View reviewed changes
lib/inline_svg/transform_pipeline/transformations.rb
@@ -65,7 +64,7 @@ def self.without_empty_values(params)
def self.all_default_values
custom_transformations
.values
.select { |opt| opt[:default_value] != nil }
.reject { |opt| opt[:default_value].nil? }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Manual fix 2: looks safe to me

tagliala
tagliala commented Oct 10, 2024
View reviewed changes
lib/inline_svg.rb
end
rescue NameError
raise InlineSvg::Configuration::Invalid.new("asset_file should implement the #named method")
end
Copy link
Contributor Author

@tagliala tagliala Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

suggestion: review with "hide whitespaces"

@tagliala tagliala force-pushed the chore/rubocop-stage-iv branch 3 times, most recently from 85766aa to 609a448 Compare October 11, 2024 06:45
@tagliala
Copy link
Contributor Author

@jamesmartin rebased and ready for review

Most of the changes are related to non-production code

There are two changes on production code with a manual fix, they have a comment and I'm quite positive they are safe

jamesmartin
jamesmartin reviewed Oct 14, 2024
View reviewed changes
Copy link
Owner

@jamesmartin jamesmartin left a comment

Choose a reason for hiding this comment

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

Looks good overall. Not 100% sold on swapping inject for each_with_object. Keen to understand what the motivation for that change is. 🤔

lib/inline_svg/transform_pipeline/transformations.rb Outdated Show resolved Hide resolved
@tagliala
Autofix some safe RuboCop offenses
40c7a16 
- `Layout/DotPosition`
- `Lint/AmbiguousOperator`
- `Lint/LiteralInInterpolation`
- `RSpec/BeEmpty`
- `RSpec/ContextMethod`
- `RSpec/HookArgument`
- `RSpec/MatchArray`
- `RSpec/NotToNot`
- `Style/ExpandPathArguments`
- `Style/HashSyntax`
- `Style/NestedParenthesizedCalls`
- `Style/RedundantBegin`
- `Style/RedundantConstantBase`
- `Style/RedundantReturn`
- `Style/RegexpLiteral`

Some manual fixes:
- `Lint/RedundantSplatExpansion` and subsequent unsafe
  manual fix of `Style/HashConversion`
- `Style/NonNilCheck` and subsequent switch unsafe manual fix of
  `Style/InverseMethods`
@tagliala tagliala force-pushed the chore/rubocop-stage-iv branch from 609a448 to 40c7a16 Compare October 14, 2024 07:38
@tagliala tagliala requested a review from jamesmartin October 14, 2024 15:46
@tagliala tagliala mentioned this pull request Oct 14, 2024
Merged
4 tasks
@tagliala
Copy link
Contributor Author

tagliala commented Nov 21, 2024 •
edited
Loading

Hi @jamesmartin, sorry for the bump

I did the requested change, I think this can be merged, there are very few changes to production code.

Then we can proceed by enabling frozen string literals, which should also save memory and improve performance (even if there are no benchmarks at the moment, I will try to do implement something: require_relative helps a lot because it will allow to use https://rubygems.org/gems/gem_bench and compare the same gem on local machine side to side)

I would like to test inline_svg for a project I have in mind from a long time (streamline Font Awesome usage in Rails applications)

jamesmartin
jamesmartin approved these changes Nov 21, 2024
View reviewed changes
Copy link
Owner

@jamesmartin jamesmartin left a comment

Choose a reason for hiding this comment

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

Thanks for incorporating the feedback, @tagliala. ✨

@jamesmartin jamesmartin merged commit 01b5e60 into jamesmartin:master Nov 21, 2024
7 checks passed
@tagliala tagliala deleted the chore/rubocop-stage-iv branch November 21, 2024 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamesmartin jamesmartin jamesmartin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tagliala @jamesmartin