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

feat: fix rubocop todos #338

Merged
merged 7 commits into from
Dec 9, 2022
Merged

feat: fix rubocop todos #338

merged 7 commits into from
Dec 9, 2022

Conversation

thestelz
Copy link
Contributor

@thestelz thestelz commented Oct 14, 2022

  • fix Layout/ArgumentAlignment

  • fix Layout/EmptyLineBetweenDefs

  • fix Layout/ExtraSpacing

  • fix Layout/FirstArrayElementIndentation

  • fix Layout/MultilineMethodCallBraceLayout

  • fix Layout/MultilineMethodCallIndentation

  • fix Layout/SpaceAroundOperators

  • fix Layout/SpaceBeforeBlockBraces

  • fix Lint/EmptyWhen

  • fix Lint/LiteralAsCondition

  • fix Lint/UnusedBlockArgument

  • fix Style/BlockDelimiters

  • fix Style/CaseLikeIf

  • fix Style/ClassEqualityComparison

  • fix Style/ConditionalAssignment

  • fix Style/EmptyMethod

  • fix Style/IfInsideElse

  • fix Style/MultilineTernaryOperator

  • fix Style/OrAssignment

  • fix Style/RedundantConditional

  • fix Style/RedundantRegexpEscape

  • fix Style/StringConcatenation

  • fix Style/StringLiterals

  • fix Style/SymbolArray

  • fix Style/PerlBackrefs

  • fix Naming/AccessorMethodName

  • fix Naming/MemoizedInstanceVariableName

  • fix Style/NegatedIfElseCondition

  • fix Style/NestedTernaryOperator

  • fix Lint/FloatComparison

  • fix Lint/DuplicateBranch

  • fix Lint/UnusedMethodArgument

  • fix Naming/BinaryOperatorParameterName

  • fix Style/AccessorGrouping

  • fix Style/InverseMethods

  • fix Style/IfUnlessModifier

  • fix Style/MultilineIfModifier

  • fix Style/MultipleComparison

  • fix Style/SoleNestedConditional

  • fix Style/UnpackFirst

  • fix Security/MarshalLoad

  • fix Style/GuardClause

  • fix Style/MultilineBlockChain

  • partially fix Metrics/BlockNesting

  • fix Layout/LineLength

  • fix Style/GlobalVars

  • Change head to 3.1 for Memcheck

  • Lock jruby version to fix bug

  • Add history line

updates #336


Adding the initial PR so if anyone wants to let me know I'm going against any styles or something like that. I'm updating the code the way I would do it while also falling within the rubocop settings/rules.

thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 14, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 14, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 14, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 14, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 14, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 14, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 14, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Oct 17, 2022
@thestelz
Copy link
Contributor Author

Unfortunately it seems that you forgot most of the if conditions in the tests.

Sorry! Forgot about fixing those. I'm not sure how to fix all of them, so I reverted the ones I wasn't sure about back and will be adding comments in the code on how to fix.

thestelz added a commit to thestelz/rgeo that referenced this pull request Nov 16, 2022
Copy link
Member

@keithdoggett keithdoggett left a comment

Choose a reason for hiding this comment

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

@thestelz @BuonOmo this all looks good to me, thanks for all the effort put in on this! Of course there's a lot here, but I didn't see anything that looked obviously incorrect or like something that should be changed.

I suppose that the points about still returning nil in some instances is not really relevant to this PR, but we should probably address them @BuonOmo.

@keithdoggett
Copy link
Member

@thestelz WRT the Style/MultilineIfModifier issue in the tests, I think that adding a setup method with a skip like you mentioned is a fine option.

thestelz added a commit to thestelz/rgeo that referenced this pull request Nov 29, 2022
Copy link
Member

@BuonOmo BuonOmo left a comment

Choose a reason for hiding this comment

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

LGTM!

I thought of one potential patch, if you're in I'll merge it later on, in the mean time I'm merging :)

diff --git a/test/geos_capi/factory_test.rb b/test/geos_capi/factory_test.rb
index 3572a9b..fda260f 100644
--- a/test/geos_capi/factory_test.rb
+++ b/test/geos_capi/factory_test.rb
@@ -7,12 +7,13 @@
 # -----------------------------------------------------------------------------
 
 require "test_helper"
+require_relative "skip_capi"
 
 class GeosFactoryTest < Minitest::Test # :nodoc:
   include RGeo::Tests::Common::FactoryTests
+  prepend SkipCAPI
 
   def setup
-    skip "Needs GEOS CAPI." unless RGeo::Geos.capi_supported?
     @factory = RGeo::Geos.factory(srid: 1000)
     @srid = 1000
   end
diff --git a/test/geos_capi/geometry_collection_test.rb b/test/geos_capi/geometry_collection_test.rb
index 0fc5eec..d25c2cf 100644
--- a/test/geos_capi/geometry_collection_test.rb
+++ b/test/geos_capi/geometry_collection_test.rb
@@ -7,15 +7,11 @@
 # -----------------------------------------------------------------------------
 
 require_relative "../test_helper"
+require_relative "skip_capi"
 
 class GeosGeometryCollectionTest < Minitest::Test # :nodoc:
   include RGeo::Tests::Common::GeometryCollectionTests
-
-  def setup
-    skip "Needs GEOS CAPI." unless RGeo::Geos.capi_supported?
-
-    super
-  end
+  prepend SkipCAPI
 
   def create_factory
     RGeo::Geos.factory
diff --git a/test/geos_capi/misc_test.rb b/test/geos_capi/misc_test.rb
index 017776e..d67d73e 100644
--- a/test/geos_capi/misc_test.rb
+++ b/test/geos_capi/misc_test.rb
@@ -9,6 +9,7 @@
 require "ostruct"
 require_relative "../test_helper"
 require_relative "../common/validity_tests"
+require_relative "skip_capi"
 
 class GeosMiscTest < Minitest::Test # :nodoc:
   def setup
diff --git a/test/geos_capi/multi_line_string_test.rb b/test/geos_capi/multi_line_string_test.rb
index 8541244..b499360 100644
--- a/test/geos_capi/multi_line_string_test.rb
+++ b/test/geos_capi/multi_line_string_test.rb
@@ -7,15 +7,11 @@
 # -----------------------------------------------------------------------------
 
 require "test_helper"
+require_relative "skip_capi"
 
 class GeosMultiLineStringTest < Minitest::Test # :nodoc:
   include RGeo::Tests::Common::MultiLineStringTests
-
-  def setup
-    skip "Needs GEOS CAPI." unless RGeo::Geos.capi_supported?
-
-    super
-  end
+  prepend SkipCAPI
 
   def create_factory
     RGeo::Geos.factory
diff --git a/test/geos_capi/multi_point_test.rb b/test/geos_capi/multi_point_test.rb
index 03d9742..4ce8548 100644
--- a/test/geos_capi/multi_point_test.rb
+++ b/test/geos_capi/multi_point_test.rb
@@ -7,15 +7,11 @@
 # -----------------------------------------------------------------------------
 
 require "test_helper"
+require_relative "skip_capi"
 
 class GeosMultiPointTest < Minitest::Test # :nodoc:
   include RGeo::Tests::Common::MultiPointTests
-
-  def setup
-    skip "Needs GEOS CAPI." unless RGeo::Geos.capi_supported?
-
-    super
-  end
+  prepend SkipCAPI
 
   def create_factory(opts = {})
     RGeo::Geos.factory(opts)
diff --git a/test/geos_capi/multi_polygon_test.rb b/test/geos_capi/multi_polygon_test.rb
index 99be47d..ecb0b4b 100644
--- a/test/geos_capi/multi_polygon_test.rb
+++ b/test/geos_capi/multi_polygon_test.rb
@@ -7,15 +7,11 @@
 # -----------------------------------------------------------------------------
 
 require "test_helper"
+require_relative "skip_capi"
 
 class GeosMultiPolygonTest < Minitest::Test # :nodoc:
   include RGeo::Tests::Common::MultiPolygonTests
-
-  def setup
-    skip "Needs GEOS CAPI." unless RGeo::Geos.capi_supported?
-
-    super
-  end
+  prepend SkipCAPI
 
   def create_factories
     @factory = RGeo::Geos.factory
diff --git a/test/geos_capi/point_test.rb b/test/geos_capi/point_test.rb
index 4303ff7..60e4eb6 100644
--- a/test/geos_capi/point_test.rb
+++ b/test/geos_capi/point_test.rb
@@ -7,12 +7,13 @@
 # -----------------------------------------------------------------------------
 
 require "test_helper"
+require_relative "skip_capi"
 
 class GeosPointTest < Minitest::Test # :nodoc:
   include RGeo::Tests::Common::PointTests
+  prepend SkipCAPI
 
   def setup
-    skip "Needs GEOS CAPI." unless RGeo::Geos.capi_supported?
     @factory = RGeo::Geos.factory(buffer_resolution: 8)
     @zfactory = RGeo::Geos.factory(has_z_coordinate: true)
     @mfactory = RGeo::Geos.factory(has_m_coordinate: true)
diff --git a/test/geos_capi/polygon_test.rb b/test/geos_capi/polygon_test.rb
index 9863466..82b26ed 100644
--- a/test/geos_capi/polygon_test.rb
+++ b/test/geos_capi/polygon_test.rb
@@ -7,19 +7,20 @@
 # -----------------------------------------------------------------------------
 
 require_relative "../test_helper"
+require_relative "skip_capi"
 
 class GeosPolygonTest < Minitest::Test # :nodoc:
   include RGeo::Tests::Common::PolygonTests
-
-  def assert_close_enough(pt1, pt2)
-    assert((pt1.x - pt2.x).abs < 0.00000001 && (pt1.y - pt2.y).abs < 0.00000001)
-  end
+  prepend SkipCAPI
 
   def setup
-    skip "Needs GEOS CAPI." unless RGeo::Geos.capi_supported?
     @factory = RGeo::Geos.factory
   end
 
+  def assert_close_enough(pt1, pt2)
+    assert((pt1.x - pt2.x).abs < 0.00000001 && (pt1.y - pt2.y).abs < 0.00000001)
+  end
+
   def test_intersection
     point1 = @factory.point(0, 0)
     point2 = @factory.point(0, 2)
diff --git a/test/geos_capi/skip_capi.rb b/test/geos_capi/skip_capi.rb
new file mode 100644
index 0000000..4cb2269
--- /dev/null
+++ b/test/geos_capi/skip_capi.rb
@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+module SkipCAPI
+  def setup
+    skip "Needs GEOS CAPI." unless RGeo::Geos.capi_supported?
+    super
+  end
+end
diff --git a/test/geos_capi/validity_test.rb b/test/geos_capi/validity_test.rb
index 78ac5fe..7e918df 100644
--- a/test/geos_capi/validity_test.rb
+++ b/test/geos_capi/validity_test.rb
@@ -7,12 +7,13 @@
 # -----------------------------------------------------------------------------
 
 require_relative "../test_helper"
+require_relative "skip_capi"
 
 class GeosValidityTest < Minitest::Test # :nodoc:
   include RGeo::Tests::Common::ValidityTests
+  prepend SkipCAPI
 
   def setup
-    skip "Needs GEOS CAPI." unless RGeo::Geos.capi_supported?
     @factory = RGeo::Geos.factory
   end
 
diff --git a/test/geos_capi/zmfactory_test.rb b/test/geos_capi/zmfactory_test.rb
index 38dfd62..7ba242e 100644
--- a/test/geos_capi/zmfactory_test.rb
+++ b/test/geos_capi/zmfactory_test.rb
@@ -7,12 +7,13 @@
 # -----------------------------------------------------------------------------
 
 require "test_helper"
+require_relative "skip_capi"
 
 class GeosZMFactoryTest < Minitest::Test # :nodoc:
   include RGeo::Tests::Common::FactoryTests
+  prepend SkipCAPI
 
   def setup
-    skip "Needs GEOS CAPI." unless RGeo::Geos.capi_supported?
     @factory = RGeo::Geos.factory(has_z_coordinate: true, has_m_coordinate: true, srid: 1000, buffer_resolution: 2)
     @srid = 1000
   end
diff --git a/test/geos_ffi/factory_test.rb b/test/geos_ffi/factory_test.rb
index f84b314..e42786d 100644
--- a/test/geos_ffi/factory_test.rb
+++ b/test/geos_ffi/factory_test.rb
@@ -7,13 +7,13 @@
 # -----------------------------------------------------------------------------
 
 require_relative "../test_helper"
+require_relative "skip_ffi"
 
 class GeosFFIFactoryTest < Minitest::Test # :nodoc:
   include RGeo::Tests::Common::FactoryTests
+  include SkipFFI
 
   def setup
-    skip "Needs GEOS FFI." unless RGeo::Geos.ffi_supported?
-
     @factory = RGeo::Geos.factory(srid: 1000, native_interface: :ffi)
     @srid = 1000
   end
diff --git a/test/geos_ffi/geometry_collection_test.rb b/test/geos_ffi/geometry_collection_test.rb
index 86600a1..2f79c57 100644
--- a/test/geos_ffi/geometry_collection_test.rb
+++ b/test/geos_ffi/geometry_collection_test.rb
@@ -7,15 +7,11 @@
 # -----------------------------------------------------------------------------
 
 require "test_helper"
+require_relative "skip_ffi"
 
 class GeosFFIGeometryCollectionTest < Minitest::Test # :nodoc:
   include RGeo::Tests::Common::GeometryCollectionTests
-
-  def setup
-    skip "Needs GEOS FFI." unless RGeo::Geos.ffi_supported?
-
-    super
-  end
+  include SkipFFI
 
   def create_factory
     RGeo::Geos.factory(native_interface: :ffi)
diff --git a/test/geos_ffi/line_string_test.rb b/test/geos_ffi/line_string_test.rb
index 38a07ae..d0902c1 100644
--- a/test/geos_ffi/line_string_test.rb
+++ b/test/geos_ffi/line_string_test.rb
@@ -7,13 +7,13 @@
 # -----------------------------------------------------------------------------
 
 require_relative "../test_helper"
+require_relative "skip_ffi"
 
 class GeosFFILineStringTest < Minitest::Test # :nodoc:
   include RGeo::Tests::Common::LineStringTests
+  include SkipFFI
 
   def setup
-    skip "Needs GEOS FFI." unless RGeo::Geos.ffi_supported?
-
     @factory = RGeo::Geos.factory(native_interface: :ffi)
   end
 end
diff --git a/test/geos_ffi/misc_test.rb b/test/geos_ffi/misc_test.rb
index 8eb45c4..c918629 100644
--- a/test/geos_ffi/misc_test.rb
+++ b/test/geos_ffi/misc_test.rb
@@ -8,11 +8,12 @@
 
 require_relative "../test_helper"
 require_relative "../common/validity_tests"
+require_relative "skip_ffi"
 
 class GeosFFIMiscTest < Minitest::Test # :nodoc:
-  def setup
-    skip "Needs GEOS FFI." unless RGeo::Geos.ffi_supported?
+  prepend SkipFFI
 
+  def setup
     @factory = RGeo::Geos.factory(srid: 4326, native_interface: :ffi)
   end
 
diff --git a/test/geos_ffi/multi_line_string_test.rb b/test/geos_ffi/multi_line_string_test.rb
index 4f1c94c..454fc33 100644
--- a/test/geos_ffi/multi_line_string_test.rb
+++ b/test/geos_ffi/multi_line_string_test.rb
@@ -7,15 +7,11 @@
 # -----------------------------------------------------------------------------
 
 require "test_helper"
+require_relative "skip_ffi"
 
 class GeosFFIMultiLineStringTest < Minitest::Test # :nodoc:
   include RGeo::Tests::Common::MultiLineStringTests
-
-  def setup
-    skip "Needs GEOS FFI." unless RGeo::Geos.ffi_supported?
-
-    super
-  end
+  include SkipFFI
 
   def create_factory
     RGeo::Geos.factory(native_interface: :ffi)
diff --git a/test/geos_ffi/multi_point_test.rb b/test/geos_ffi/multi_point_test.rb
index 3643e65..ffcaa99 100644
--- a/test/geos_ffi/multi_point_test.rb
+++ b/test/geos_ffi/multi_point_test.rb
@@ -7,15 +7,11 @@
 # -----------------------------------------------------------------------------
 
 require "test_helper"
+require_relative "skip_ffi"
 
 class GeosFFIMultiPointTest < Minitest::Test # :nodoc:
   include RGeo::Tests::Common::MultiPointTests
-
-  def setup
-    skip "Needs GEOS FFI." unless RGeo::Geos.ffi_supported?
-
-    super
-  end
+  include SkipFFI
 
   def create_factory(opts = {})
     RGeo::Geos.factory(opts.merge(native_interface: :ffi))
diff --git a/test/geos_ffi/multi_polygon_test.rb b/test/geos_ffi/multi_polygon_test.rb
index 8673c7f..8e30fa3 100644
--- a/test/geos_ffi/multi_polygon_test.rb
+++ b/test/geos_ffi/multi_polygon_test.rb
@@ -7,15 +7,11 @@
 # -----------------------------------------------------------------------------
 
 require "test_helper"
+require_relative "skip_ffi"
 
 class GeosFFIMultiPolygonTest < Minitest::Test # :nodoc:
   include RGeo::Tests::Common::MultiPolygonTests
-
-  def setup
-    skip "Needs GEOS FFI." unless RGeo::Geos.ffi_supported?
-
-    super
-  end
+  include SkipFFI
 
   def create_factories
     @factory = RGeo::Geos.factory(native_interface: :ffi)
diff --git a/test/geos_ffi/point_test.rb b/test/geos_ffi/point_test.rb
index ed5a6a4..774cb27 100644
--- a/test/geos_ffi/point_test.rb
+++ b/test/geos_ffi/point_test.rb
@@ -7,13 +7,13 @@
 # -----------------------------------------------------------------------------
 
 require "test_helper"
+require_relative "skip_ffi"
 
 class GeosFFIPointTest < Minitest::Test # :nodoc:
   include RGeo::Tests::Common::PointTests
+  include SkipFFI
 
   def setup
-    skip "Needs GEOS FFI." unless RGeo::Geos.ffi_supported?
-
     @factory = RGeo::Geos.factory(native_interface: :ffi, buffer_resolution: 8)
     @zfactory = RGeo::Geos.factory(has_z_coordinate: true, native_interface: :ffi)
     @mfactory = RGeo::Geos.factory(has_m_coordinate: true, native_interface: :ffi)
diff --git a/test/geos_ffi/polygon_test.rb b/test/geos_ffi/polygon_test.rb
index ac0238e..5efb99f 100644
--- a/test/geos_ffi/polygon_test.rb
+++ b/test/geos_ffi/polygon_test.rb
@@ -7,13 +7,13 @@
 # -----------------------------------------------------------------------------
 
 require "test_helper"
+require_relative "skip_ffi"
 
 class GeosFFIPolygonTest < Minitest::Test # :nodoc:
   include RGeo::Tests::Common::PolygonTests
+  include SkipFFI
 
   def setup
-    skip "Needs GEOS FFI." unless RGeo::Geos.ffi_supported?
-
     @factory = RGeo::Geos.factory(native_interface: :ffi)
   end
 
diff --git a/test/geos_ffi/skip_ffi.rb b/test/geos_ffi/skip_ffi.rb
new file mode 100644
index 0000000..4eccbc9
--- /dev/null
+++ b/test/geos_ffi/skip_ffi.rb
@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+module SkipFFI
+  def setup
+    skip "Needs GEOS FFI." unless RGeo::Geos.ffi_supported?
+    super
+  end
+end
diff --git a/test/geos_ffi/validity_test.rb b/test/geos_ffi/validity_test.rb
index 683b69e..369c5cb 100644
--- a/test/geos_ffi/validity_test.rb
+++ b/test/geos_ffi/validity_test.rb
@@ -7,13 +7,13 @@
 # -----------------------------------------------------------------------------
 
 require_relative "../test_helper"
+require_relative "skip_ffi"
 
 class GeosFFIValidityTest < Minitest::Test # :nodoc:
   include RGeo::Tests::Common::ValidityTests
+  include SkipFFI
 
   def setup
-    skip "Needs GEOS FFI." unless RGeo::Geos.ffi_supported?
-
     @factory = RGeo::Geos.factory(native_interface: :ffi)
   end
 end
diff --git a/test/geos_ffi/wkrep_test.rb b/test/geos_ffi/wkrep_test.rb
index 6fa6a7b..d768f87 100644
--- a/test/geos_ffi/wkrep_test.rb
+++ b/test/geos_ffi/wkrep_test.rb
@@ -1,10 +1,12 @@
 # frozen_string_literal: true
 
 require_relative "../test_helper"
+require_relative "skip_ffi"
 
 class GeosFFIWKREPTest < Minitest::Test
+  include SkipFFI
+
   def setup
-    skip "Needs GEOS FFI." unless RGeo::Geos.ffi_supported?
     @factory = RGeo::Geos.factory(native_interface: :ffi)
   end
 
diff --git a/test/geos_ffi/zmfactory_test.rb b/test/geos_ffi/zmfactory_test.rb
index a40de89..2b2c903 100644
--- a/test/geos_ffi/zmfactory_test.rb
+++ b/test/geos_ffi/zmfactory_test.rb
@@ -7,13 +7,13 @@
 # -----------------------------------------------------------------------------
 
 require "test_helper"
+require_relative "skip_ffi"
 
 class GeosFFIZMFactoryTest < Minitest::Test # :nodoc:
   include RGeo::Tests::Common::FactoryTests
+  include SkipFFI
 
   def setup
-    skip "Needs GEOS FFI." unless RGeo::Geos.ffi_supported?
-
     @factory = RGeo::Geos.factory(has_z_coordinate: true, has_m_coordinate: true,
                                   srid: 1000, buffer_resolution: 2, native_interface: :ffi)
     @srid = 1000

@BuonOmo
Copy link
Member

BuonOmo commented Dec 4, 2022

Oh the CI is failing... I'll look at that later today hopefully. Besides that, @thestelz would you like to add a History.md entry for your work ? You clearly deserve it!

@thestelz
Copy link
Contributor Author

thestelz commented Dec 5, 2022

I thought of one potential patch, if you're in I'll merge it later on, in the mean time I'm merging :)

I like it and makes sense to me!

Oh the CI is failing... I'll look at that later today hopefully.

@BuonOmo ya, I need to look into why that is happening (since I don't have jruby locally, I have to test via github). I meant to do that over the weekend, but ran out of time. I'll try and get to it this week.

Besides that, @thestelz would you like to add a History.md entry for your work ? You clearly deserve it!

Can do!

thestelz added a commit to thestelz/rgeo that referenced this pull request Dec 5, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Dec 6, 2022
@thestelz
Copy link
Contributor Author

thestelz commented Dec 6, 2022

@BuonOmo I'm not sure why the test is failing. I don't recall seeing that error before and I don't have any experience with jruby, so I'm not sure how to fix.

Also, how would you like me to write up the history and what section should I put it in?

@keithdoggett
Copy link
Member

keithdoggett commented Dec 6, 2022

@thestelz I believe the solution to the failing CI is to pin JRuby to an older version. See my comment here (rgeo/rgeo-activerecord#72 (comment)). There seems to be an unrelated bug with JRuby 9.4+.

So in CI.yml in the ruby matrix change jruby to "jruby-9.3.7.0"

@keithdoggett
Copy link
Member

Also, how would you like me to write up the history and what section should I put it in?

Under Current and Minor Changes in History, add a line with something like:

* Fix rubocop_todos and other refactoring. (thestelz) #338 

thestelz added a commit to thestelz/rgeo that referenced this pull request Dec 6, 2022
* Lock jruby version to fix bug

* Add history line

updates rgeo#336, rgeo#338
@keithdoggett
Copy link
Member

@thestelz can you try changing the ruby-version in the Memcheck part of the CI to 3.1 instead of head? The error message there seems to indicate it's having trouble with the Ruby 3.2 image for some reason.

thestelz added a commit to thestelz/rgeo that referenced this pull request Dec 6, 2022
thestelz added a commit to thestelz/rgeo that referenced this pull request Dec 6, 2022
@thestelz
Copy link
Contributor Author

thestelz commented Dec 6, 2022

@keithdoggett Thanks for your help! That has solved it. Would you like me to squash my commits down or will you guys do that on merging? (Also, if you have a specific way of doing your commits and you want me to squash them, let me know. If not, I'll write it up like we do in our company.)

@keithdoggett
Copy link
Member

@thestelz thank you for all the work on this! We don't have a standard way we squash commits and haven't really done it much on this project.

@BuonOmo do you have an opinion on this? I suppose we could handle it ourselves or just merge as is.

@BuonOmo
Copy link
Member

BuonOmo commented Dec 7, 2022

I usually use the squash and merge button. But if you want to squash yourself and write the commit message as you'd like to have it, you can. There is no rule yet on that topic.

I guess when we'll discuss that further we could consider conventionnal commits :)

* fix `Layout/ArgumentAlignment`

* fix `Layout/EmptyLineBetweenDefs`

* fix `Layout/ExtraSpacing`

* fix `Layout/FirstArrayElementIndentation`

* fix `Layout/MultilineMethodCallBraceLayout`

* fix `Layout/MultilineMethodCallIndentation`

* fix `Layout/SpaceAroundOperators`

* fix `Layout/SpaceBeforeBlockBraces`

* fix `Lint/EmptyWhen`

* fix `Lint/LiteralAsCondition`

* fix `Lint/UnusedBlockArgument`

* fix `Style/BlockDelimiters`

* fix `Style/CaseLikeIf`

* fix `Style/ClassEqualityComparison`

* fix `Style/ConditionalAssignment`

* fix `Style/EmptyMethod`

* fix `Style/IfInsideElse`

* fix `Style/MultilineTernaryOperator`

* fix `Style/OrAssignment`

* fix `Style/RedundantConditional`

* fix `Style/RedundantRegexpEscape`

* fix `Style/StringConcatenation`

* fix `Style/StringLiterals`

* fix `Style/SymbolArray`

* fix `Style/PerlBackrefs`

* fix `Naming/AccessorMethodName`

* fix `Naming/MemoizedInstanceVariableName`

* fix `Style/NegatedIfElseCondition`

* fix `Style/NestedTernaryOperator`

* fix `Lint/FloatComparison`

* fix `Lint/DuplicateBranch`

* fix `Lint/UnusedMethodArgument`

* fix `Naming/BinaryOperatorParameterName`

* fix `Style/AccessorGrouping`

* fix `Style/InverseMethods`

* fix `Style/IfUnlessModifier`

* fix `Style/MultilineIfModifier`

* fix `Style/MultipleComparison`

* fix `Style/SoleNestedConditional`

* fix `Style/UnpackFirst`

* fix `Security/MarshalLoad`

* fix `Style/GuardClause`

* fix `Style/MultilineBlockChain`

* partially fix `Metrics/BlockNesting`

* fix `Layout/LineLength`

* fix `Style/GlobalVars`

* Change head to 3.1 for Memcheck

* Lock jruby version to fix bug

* Add history line

updates rgeo#336
@thestelz thestelz changed the title feat: remove/fix rubocop todos feat: fix rubocop todos Dec 8, 2022
@thestelz
Copy link
Contributor Author

thestelz commented Dec 8, 2022

@BuonOmo and @keithdoggett I have everything squashed as much as I could get it (I back-merged main, so I can't squash past that -- you might be able to do that via GitHub tho). If there is anything else let me know.

@BuonOmo BuonOmo merged commit 251466a into rgeo:main Dec 9, 2022
@BuonOmo
Copy link
Member

BuonOmo commented Dec 9, 2022

Squashed & Merged, thank you @thestelz ! 🎉

@thestelz
Copy link
Contributor Author

thestelz commented Dec 9, 2022

@BuonOmo & @keithdoggett thanks for your help!

@thestelz thestelz deleted the rubocop_fixes.g336 branch December 9, 2022 16:52
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

3 participants