Terse Rule: fix inferred id not generating unique suffix (#264)

Signed-off-by: Jimmy Tanagra <[email protected]>
openhab · Mar 19, 2024 · bfeb173 · bfeb173
1 parent 609dc70
commit bfeb173
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 18 deletions.
diff --git a/lib/openhab/dsl/rules/builder.rb b/lib/openhab/dsl/rules/builder.rb
@@ -73,7 +73,7 @@ def initialize(provider)
         #     run { logger.info "Happy new day!" }
         #   end
         #
-        def rule(name = nil, id: nil, replace: false, script: nil, binding: nil, &block)
+        def rule(name = nil, id: nil, replace: nil, script: nil, binding: nil, &block)
           raise ArgumentError, "Block is required" unless block
 
           inferred_id = nil
@@ -89,7 +89,7 @@ def rule(name = nil, id: nil, replace: false, script: nil, binding: nil, &block)
 
             if replace
               logger.debug { "Removing existing rule '#{builder.uid}'." } if DSL.rules.remove(builder.uid)
-            else
+            elsif replace.nil?
               id_not_inferred = inferred_id.nil? || inferred_id != builder.uid
               if id_not_inferred && (existing_rule = $rules.get(builder.uid))
                 logger.warn "Rule '#{builder.uid}' is not created because " \

diff --git a/lib/openhab/dsl/rules/terse.rb b/lib/openhab/dsl/rules/terse.rb
@@ -44,22 +44,27 @@ class << self
           #   @see BuilderDSL#$1
           def def_terse_rule(trigger)
             class_eval(<<~RUBY, __FILE__, __LINE__ + 1)
-              def #{trigger}(*args, id: nil, name: nil, description: nil,       # def changed(*args, id: nil, name: nil, description: nil,
-                  tag: nil, tags: nil, on_load: false, **kwargs, &block)        #     tag: nil, tags: nil, on_load: false, **kwargs, &block)
-                raise ArgumentError, "Block is required" unless block           #   raise ArgumentError, "Block is required" unless block
-                                                                                #
-                id ||= NameInference.infer_rule_id_from_block(block)            #   id ||= NameInference.infer_rule_id_from_block(block)
-                script = block.source rescue nil                                #   script = block.source rescue nil
-                caller_binding = block.binding                                  #   caller_binding = block.binding
-                rule name, id: id, script: script, binding: caller_binding do   #   rule name, id: id, script: script, binding: caller_binding do
-                  self.on_load if on_load                                       #     self.on_load if on_load
-                  self.description(description) if description                  #     self.description(description) if description
-                  self.tags(*Array.wrap(tag), *Array.wrap(tags))                #     self.tags(*Array.wrap(tag), *Array.wrap(tags))
-                  #{trigger}(*args, **kwargs)                                   #     changed(*args, **kwargs)
-                  run(&block)                                                   #     run(&block)
-                end                                                             #   end
-              end                                                               # end
-              module_function #{trigger.inspect}                                # module_function :changed
+              def #{trigger}(*args, id: nil, name: nil, description: nil,  # def changed(*args, id: nil, name: nil, description: nil,
+                  tag: nil, tags: nil, on_load: false, **kwargs, &block)   #     tag: nil, tags: nil, on_load: false, **kwargs, &block)
+                raise ArgumentError, "Block is required" unless block      #   raise ArgumentError, "Block is required" unless block
+                                                                           #
+                replace = nil                                              #   replace = nil
+                unless id                                                  #   unless id
+                  replace = false                                          #     replace = false
+                  id = NameInference.infer_rule_id_from_block(block)       #     id = NameInference.infer_rule_id_from_block(block)
+                end                                                        #   end
+                script = block.source rescue nil                           #   script = block.source rescue nil
+                caller_binding = block.binding                             #   caller_binding = block.binding
+                rule name, id: id, replace: replace, script: script,       #   rule name, id: id, replace: replace, script: script
+                  binding: caller_binding do                               #     binding: caller_binding do
+                  self.on_load if on_load                                  #     self.on_load if on_load
+                  self.description(description) if description             #     self.description(description) if description
+                  self.tags(*Array.wrap(tag), *Array.wrap(tags))           #     self.tags(*Array.wrap(tag), *Array.wrap(tags))
+                  #{trigger}(*args, **kwargs)                              #     changed(*args, **kwargs)
+                  run(&block)                                              #     run(&block)
+                end                                                        #   end
+              end                                                          # end
+              module_function #{trigger.inspect}                           # module_function :changed
             RUBY
           end
         end

diff --git a/spec/openhab/dsl/rules/terse_spec.rb b/spec/openhab/dsl/rules/terse_spec.rb
@@ -16,6 +16,20 @@
     expect(ran).to be true
   end
 
+  it "infers a unique id for each rule" do
+    rules = []
+    2.times do
+      rules << changed(TestSwitch) { nil }
+    end
+    expect(rules.map(&:uid).uniq.length).to be > 1
+  end
+
+  it "doesn't create a new rule with an existing rule id" do
+    changed(TestSwitch, id: "Test") { nil }
+    rule = changed(TestSwitch, id: "Test") { nil }
+    expect(rule).to be_nil
+  end
+
   it "returns the rule object" do
     rule = changed(TestSwitch) { nil }
     expect(rule).to be_a OpenHAB::Core::Rules::Rule