Fix #148 Add cop for Entity new #161

Eneroth3 · 2023-03-17T15:18:13Z

Work in progress.

My node matcher ((send (const (const nil? :Sketchup) :Face) :new _)) doesn't seem to catch anything.

For now it should catch Sketchup::Face.new and skip Edge and Group, but all 3 tests give identical results.

Failures:

  1) RuboCop::Cop::SketchupRequirements::InitEntity registers an offense when using `Sketchup::Edge.new`
     Failure/Error:
       expect_offense(<<~RUBY)
         Sketchup::Edge.new
         ^^^^^^^^^^^^^^^^^^ Entity objects (Face, Edge, Group etc) are managed SketchUp. Instead of using the `new` constructor, use the `Sketchup::Entities#add_*` methods.
       RUBY

       Diff:
       @@ -1,3 +1,2 @@
        Sketchup::Edge.new
       -^^^^^^^^^^^^^^^^^^ Entity objects (Face, Edge, Group etc) are managed SketchUp. Instead of using the `new` constructor, use the `Sketchup::Entities#add_*` methods.

     # ./spec/rubocop/cop/sketchup_requirements/init_entity_spec.rb:17:in `block (2 levels) in <top (required)>'

  2) RuboCop::Cop::SketchupRequirements::InitEntity registers an offense when using `Sketchup::Group.new`
     Failure/Error:
       expect_offense(<<~RUBY)
         Sketchup::Group.new
         ^^^^^^^^^^^^^^^^^^^ Entity objects (Face, Edge, Group etc) are managed SketchUp. Instead of using the `new` constructor, use the `Sketchup::Entities#add_*` methods.
       RUBY

       Diff:
       @@ -1,3 +1,2 @@
        Sketchup::Group.new
       -^^^^^^^^^^^^^^^^^^^ Entity objects (Face, Edge, Group etc) are managed SketchUp. Instead of using the `new` constructor, use the `Sketchup::Entities#add_*` methods.

     # ./spec/rubocop/cop/sketchup_requirements/init_entity_spec.rb:24:in `block (2 levels) in <top (required)>'

  3) RuboCop::Cop::SketchupRequirements::InitEntity registers an offense when using `Sketchup::Face.new`
     Failure/Error:
       expect_offense(<<~RUBY)
         Sketchup::Face.new
         ^^^^^^^^^^^^^^^^^^ Entity objects (Face, Edge, Group etc) are managed SketchUp. Instead of using the `new` constructor, use the `Sketchup::Entities#add_*` methods.
       RUBY

       Diff:
       @@ -1,3 +1,2 @@
        Sketchup::Face.new
       -^^^^^^^^^^^^^^^^^^ Entity objects (Face, Edge, Group etc) are managed SketchUp. Instead of using the `new` constructor, use the `Sketchup::Entities#add_*` methods.

I'm suspecting a syntax issue, but I'm not sure. @thomthom, can you have a look?

thomthom · 2023-03-20T10:42:21Z

My node matcher ((send (const (const nil? :Sketchup) :Face) :new _)) doesn't seem to catch anything.

I'm guessing that it's trying to match on new as an instance method, but we want to catch usage of new as a class method.

Your matcher looks close though. (Have you found the ruby-parse tool?)

C:\Users\Thomas>ruby-parse -e 'Sketchup::Face.new'
warning: parser/current is loading parser/ruby27, which recognizes 2.7.7-compliant syntax, but you are running 2.7.4.
Please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
(send
  (const
    (const nil :Sketchup) :Face) :new)

C:\Users\Thomas>

One thing I wonder, is if the underscore in your matcher means it expects at least one param?

Btw, a matcher is capable of capturing part of the node, so that you should be able to capture the class name and then reuse that in the error message eventually.

thomthom · 2023-03-20T10:43:47Z

Yea, you might want to replace _ with ... to match any number of nodes (including zero):
https://docs.rubocop.org/rubocop-ast/node_pattern.html#for-several-subsequent-nodes

And worth to add tests for both with and without arguments passed to new.

Eneroth3 · 2023-03-20T14:50:32Z

Got it to work know and added a bunch of more classes to check for

.rubocop.yml

thomthom · 2023-03-21T10:41:15Z

config/default.yml

@@ -209,6 +209,11 @@ SketchupRequirements/GlobalVariables:
  Reference: https://github.com/SketchUp/rubocop-sketchup/tree/main/manual/cops_requirements.md#globalvariables
  Enabled: true

+SketchupRequirements/InitEntity:
+  Description: Do not init SketchUp Entity objects with `new`.


I'd expand init to initialize.

Does this need to be regenerated? bundle exec rake ?

Still this init usage.

Ah, missed the one in the description

lib/rubocop/sketchup/cop/requirements/init_entity.rb

Eneroth3 · 2023-03-21T15:25:11Z

Ready for re-review!

thomthom · 2023-03-21T16:19:40Z

lib/rubocop/sketchup/cop/requirements/initialize_entity.rb

+          (send
+            (const
+              (const nil? :Sketchup)
+              {:Face :Group :Edge :ConmponentInsstance :Image}


ConmponentInsstance => ComponentInstance

It's possible to call a method from a matcher. Syntax uses something like #methodname I think. But we could use that to make the node matcher look up a complete list of Sketchup::Entity derived classes.

https://docs.rubocop.org/rubocop-ast/node_pattern.html#to-call-functions

Added a method and also listed a whole bunch of more classes while at it.

spec/rubocop/cop/sketchup_requirements/initialize_entity_spec.rb

thomthom

Looking good. You're quickly ramping up the complexity. 👍

Spotted a couple of spelling mistakes. And got a suggestion on how we can cover all the entities instead of just a subset.

thomthom · 2023-03-22T10:48:09Z

lib/rubocop/sketchup/cop/requirements/initialise_entity.rb

+      # them.
+      class InitialiseEntity < SketchUp::Cop
+
+        ENTITY_CLASSES = %i[


Known direct subclasses:

AttributeDictionaries, AttributeDictionary, Axes, Behavior, Curve, DefinitionList, Drawingelement, EdgeUse, Layer, LayerFolder, Layers, LineStyle, LineStyles, Loop, Material, Materials, Page, Pages, RenderingOptions, ShadowInfo, Style, Styles, Texture, Vertex

And then you have the subclasses of Drawinglement:

ComponentDefinition, ComponentInstance, ConstructionLine, ConstructionPoint, Dimension, Edge, Face, Group, Image, SectionPlane, Text

thomthom · 2023-03-22T10:49:27Z

spec/rubocop/cop/sketchup_requirements/initialise_entity_spec.rb

+
+  subject(:cop) { described_class.new }
+
+  entity_classes = %i[


I'd just refer to the ENTITY_CLASSES constant in the cop here, reuse that.

thomthom · 2023-03-22T10:50:51Z

spec/rubocop/cop/sketchup_requirements/initialise_entity_spec.rb

+
+require 'spec_helper'
+
+describe RuboCop::Cop::SketchupRequirements::InitialiseEntity do


I'm sorry, I messed up last round of comments, the correct spelling in Ruby is initialize. (I don't know why I thought otherwise before.)

You are too British! :P

thomthom

LGTM (small nit: still using "init" in default.yml)

Eneroth3 force-pushed the add-entity-new-cop branch from a1cf10a to 2f5b055 Compare March 20, 2023 14:49

thomthom reviewed Mar 21, 2023

View reviewed changes

Eneroth3 force-pushed the add-entity-new-cop branch from ea02ef6 to d0c02e4 Compare March 21, 2023 15:24

thomthom reviewed Mar 21, 2023

View reviewed changes

spec/rubocop/cop/sketchup_requirements/initialize_entity_spec.rb Show resolved Hide resolved

thomthom requested changes Mar 21, 2023

View reviewed changes

Eneroth3 requested a review from thomthom March 22, 2023 10:28

thomthom reviewed Mar 22, 2023

View reviewed changes

Eneroth3 requested a review from thomthom March 22, 2023 14:21

thomthom approved these changes Mar 23, 2023

View reviewed changes

Fix #148 Add cop for Entity new

bf3ba06

Eneroth3 force-pushed the add-entity-new-cop branch from 04ea30a to bf3ba06 Compare March 23, 2023 12:30

Eneroth3 merged commit f969742 into main Mar 23, 2023

Eneroth3 deleted the add-entity-new-cop branch March 30, 2023 12:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #148 Add cop for Entity new #161

Fix #148 Add cop for Entity new #161

Eneroth3 commented Mar 17, 2023 •

edited

thomthom commented Mar 20, 2023

thomthom commented Mar 20, 2023 •

edited

Eneroth3 commented Mar 20, 2023

thomthom Mar 21, 2023

thomthom Mar 21, 2023

thomthom Mar 23, 2023

Eneroth3 Mar 23, 2023

Eneroth3 commented Mar 21, 2023

thomthom Mar 21, 2023

thomthom Mar 21, 2023

thomthom Mar 21, 2023

Eneroth3 Mar 22, 2023

thomthom left a comment

thomthom Mar 22, 2023

thomthom Mar 22, 2023

thomthom Mar 22, 2023

Eneroth3 Mar 22, 2023

thomthom left a comment


		require 'spec_helper'

		describe RuboCop::Cop::SketchupRequirements::InitialiseEntity do

Fix #148 Add cop for Entity new #161

Fix #148 Add cop for Entity new #161

Conversation

Eneroth3 commented Mar 17, 2023 • edited

thomthom commented Mar 20, 2023

thomthom commented Mar 20, 2023 • edited

Eneroth3 commented Mar 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eneroth3 commented Mar 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomthom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomthom left a comment

Choose a reason for hiding this comment

Eneroth3 commented Mar 17, 2023 •

edited

thomthom commented Mar 20, 2023 •

edited