Raise an exception when trying to subscribe to an unregister event #4

GustavoCaso · 2018-03-02T09:48:48Z

Fixes #3

@solnic @mensfeld I added the feature request to the library. Hope it helps 😄

GustavoCaso · 2018-03-14T10:38:02Z

@solnic any update on this one?

solnic · 2018-03-18T13:34:48Z

lib/dry/events/bus.rb

@@ -76,6 +76,11 @@ def subscribe(event_id, query, &block)
      def subscribed?(listener)
        listeners.values.any? { |value| value.any? { |(block, _)| block.equal?(listener) } }
      end
+
+      # @api private
+      def register_event?(event_id)


It'd be better to call this methods event_registered?

solnic · 2018-03-18T13:35:11Z

lib/dry/events/bus.rb

+
+      # @api private
+      def register_event?(event_id)
+        events.keys.any? { |value| value == event_id }


This can be simplified as events.key?(event_id)

@solnic do we store nils as values? If not it could be simplified to !events[event_id].nil? as it is faster

@mensfeld if you do register_event(nil) then it'll be stored, but we shouldn't allow that 😅

@solnic so my point is valid ;) @GustavoCaso maybe we should add a dry-validation to validate the format of the event ;)

solnic · 2018-03-18T13:35:53Z

lib/dry/events/publisher.rb

+    UnregisterEventError = Class.new(StandardError) do
+      # @api private
+      def initialize(event_id)
+        super("you are trying to subscribe to an event: #{event_id} that has not been register")


register => registered

solnic · 2018-03-18T13:36:58Z

lib/dry/events/publisher.rb

+          else
+            true
+          end
+        end


Not sure if it's OK that this returns true for other types?

Since the subscribe method accepts either an object listener or an event name we need to do a check here, I agree with the concern of returning always true for no event name, maybe we could move the check up to the subscribe method and only call this method if is an event name.

AMHOL · 2018-03-19T07:19:12Z

lib/dry/events/publisher.rb

@@ -18,6 +18,14 @@ def initialize(id)
      end
    end

+    # @api public
+    UnregisterEventError = Class.new(StandardError) do


Also another instance of unregister here

AMHOL · 2018-03-19T07:21:18Z

unregistered implies that the event was previously registered but has been removed, is this the case or is it the case for events that have never been registered? If it's the latter I'd go for something like NoEventRegisteredError and similar messages

GustavoCaso · 2018-03-20T06:20:42Z

@solnic @mensfeld @AMHOL I applied the changes. 😄

GustavoCaso · 2018-04-18T05:46:37Z

@solnic any update on this one?

GustavoCaso · 2018-04-18T05:47:03Z

Ups I accidentally close it 😄

mensfeld · 2018-04-18T08:15:50Z

Uff...

AMHOL · 2018-07-06T09:20:37Z

lib/dry/events/event.rb

+      InvalidEventNameError = Class.new(StandardError) do
+        # @api private
+        def initialize
+          super("please provide a valid event name, it could be either String or Symbol")


This seems to actually just accept any non-nil value?

AMHOL · 2018-07-06T09:20:50Z

lib/dry/events/event.rb

      DOT = '.'.freeze
      UNDERSCORE = '_'.freeze

      # @!attribute [r] id
-      #   @return [Symbol] The event identifier
+      #   @return [Symbol, String] The event identifier


AMHOL · 2018-07-06T09:21:27Z

lib/dry/events/event.rb

      # Initialize a new event
      #
-      # @param [Symbol] id The event identifier
+      # @param [Symbol, String] id The event identifier
      # @param [Hash] payload Optional payload


Says optional payload but the argument is required now

AMHOL · 2018-07-06T09:22:42Z

lib/dry/events/publisher.rb

+    NoEventRegisteredError = Class.new(StandardError) do
+      # @api private
+      def initialize(event_id)
+        super("you are trying to subscribe to an event: #{event_id} that has not been registered")


Might be worth quoting event_id with the traditional

`#{event_id}'

AMHOL · 2018-07-06T09:26:03Z

lib/dry/events/publisher.rb

+          when String, Symbol
+            __bus__.event_registered?(object_or_event_id)
+          else
+            true


How come this defaults to true?

You can subscribe to events or you could subscribe a listener object, in that case, there is no event registered in the _bus to check.

Later there is another check https://github.com/dry-rb/dry-events/blob/master/lib/dry/events/bus.rb#L53 in the Bus to see if the listener object have the appropriate method

AMHOL · 2018-07-06T09:26:38Z

spec/unit/dry/events/event_spec.rb

+      expect(ev.listener_method).to eq :on_hello_world
+    end
+  end
+end


Missing newline at EOF

AMHOL · 2018-07-06T09:26:49Z

spec/unit/dry/events/event_spec.rb

+      expect(event.listener_method).to eq :on_test
+    end
+
+    it 'replace dots for underscore' do


/s/replace/replaces

/s/underscore/underscores

matugm · 2018-11-29T18:19:08Z

I think it would also be helpful to raise an exception when trying to publish an event that doesn't exist. It's easy to make a typo & waste time looking for the problem.

mensfeld · 2018-11-30T10:17:12Z

@matugm that is a good idea. I've already wasted a lot of time because of that ;)

drqCode · 2019-02-12T15:08:39Z

This would help a lot if it will be merged and released:)

solnic · 2019-02-13T09:03:47Z

@drqCode looks like @GustavoCaso went M.I.A so somebody could pick it up, update, address issues that were mentioned in the review, and open a new PR.

mensfeld · 2019-07-03T13:10:33Z

@GustavoCaso I don't have permissions to push into your fork, so I'm closing that issue and wil reopen with your code from the origin.

GustavoCaso force-pushed the check-event-registration-before-subscribing branch from 9d3a4de to 35902e3 Compare March 2, 2018 10:30

Raise an exception when trying to subscribe to an unregister event

13a4009

GustavoCaso force-pushed the check-event-registration-before-subscribing branch from 35902e3 to 13a4009 Compare March 2, 2018 10:31

solnic requested changes Mar 18, 2018

View reviewed changes

AMHOL reviewed Mar 19, 2018

View reviewed changes

GustavoCaso added 6 commits March 20, 2018 05:55

Improve method name

d2a4e7d

Improve error class name and message

eb44602

Simplify search for event id

ec17714

Not allow nil as event_id for Event

802585d

Add specs for Dry::Events::Event

9a1998e

Improve check for event registered

49ca734

GustavoCaso closed this Apr 18, 2018

GustavoCaso reopened this Apr 18, 2018

AMHOL reviewed Jul 6, 2018

View reviewed changes

apply feedback

51fdb80

mensfeld closed this Jul 3, 2019

mensfeld mentioned this pull request Jul 3, 2019

Check event registration before subscribing #11

Merged

mensfeld mentioned this pull request Jul 12, 2019

Better exception when forgetting to register an event dry-rb/dry-monitor#31

Closed

landongrindheim mentioned this pull request Oct 4, 2020

Raise error when publishing an unregistered event #14

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise an exception when trying to subscribe to an unregister event #4

Raise an exception when trying to subscribe to an unregister event #4

GustavoCaso commented Mar 2, 2018

GustavoCaso commented Mar 14, 2018

solnic Mar 18, 2018

solnic Mar 18, 2018

mensfeld Mar 19, 2018

solnic Mar 19, 2018

mensfeld Mar 19, 2018

solnic Mar 18, 2018

solnic Mar 18, 2018

GustavoCaso Mar 20, 2018

AMHOL Mar 19, 2018

AMHOL commented Mar 19, 2018

GustavoCaso commented Mar 20, 2018

GustavoCaso commented Apr 18, 2018

GustavoCaso commented Apr 18, 2018

mensfeld commented Apr 18, 2018

AMHOL Jul 6, 2018

AMHOL Jul 6, 2018

AMHOL Jul 6, 2018

AMHOL Jul 6, 2018

AMHOL Jul 6, 2018

GustavoCaso Jul 11, 2018 •

edited

Loading

AMHOL Jul 6, 2018

AMHOL Jul 6, 2018

AMHOL Jul 6, 2018

matugm commented Nov 29, 2018

mensfeld commented Nov 30, 2018

drqCode commented Feb 12, 2019

solnic commented Feb 13, 2019

mensfeld commented Jul 3, 2019

Raise an exception when trying to subscribe to an unregister event #4

Raise an exception when trying to subscribe to an unregister event #4

Conversation

GustavoCaso commented Mar 2, 2018

GustavoCaso commented Mar 14, 2018

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

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

AMHOL commented Mar 19, 2018

GustavoCaso commented Mar 20, 2018

GustavoCaso commented Apr 18, 2018

GustavoCaso commented Apr 18, 2018

mensfeld commented Apr 18, 2018

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

GustavoCaso Jul 11, 2018 • edited Loading

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

matugm commented Nov 29, 2018

mensfeld commented Nov 30, 2018

drqCode commented Feb 12, 2019

solnic commented Feb 13, 2019

mensfeld commented Jul 3, 2019

GustavoCaso Jul 11, 2018 •

edited

Loading