Implement stop_on_error in event and handler #128

Open
spaceone opened this Issue Sep 4, 2015 · 9 comments

Comments

Projects
None yet
2 participants
@spaceone
Contributor

spaceone commented Sep 4, 2015

Event.stop_on_error and handler.stop_on_error is a very useful thing and should be implemented.

@prologic

This comment has been minimized.

Show comment
Hide comment
@prologic

prologic Sep 4, 2015

Member

Hmm an interesting idea...

Don't like the name per se (yet) but let's nut this out...

So what I think you're saying is:

  • Set Event.on_error = "abort"
  • If Event.on_error is "abort"; don't process other event handlers.

What should we do about the error thougH? hide it, log it, still fire the exception() event? Should we support other kinds of on_error behavior?

Member

prologic commented Sep 4, 2015

Hmm an interesting idea...

Don't like the name per se (yet) but let's nut this out...

So what I think you're saying is:

  • Set Event.on_error = "abort"
  • If Event.on_error is "abort"; don't process other event handlers.

What should we do about the error thougH? hide it, log it, still fire the exception() event? Should we support other kinds of on_error behavior?

@spaceone

This comment has been minimized.

Show comment
Hide comment
@spaceone

spaceone Sep 4, 2015

Contributor

yes, let's still fire the exception() event! I can't image other kinds of on_error behavior but maybe we will have ideas in the future!

Contributor

spaceone commented Sep 4, 2015

yes, let's still fire the exception() event! I can't image other kinds of on_error behavior but maybe we will have ideas in the future!

@prologic

This comment has been minimized.

Show comment
Hide comment
@prologic

prologic Sep 4, 2015

Member

Perhaps while we're at it; should we consolidate the .success, .complete and .failure attributes too? (NB:I There's a bug with the complete behavior).

Meaning we break API (for circuits 3.3):

  • on_error
  • on_success
  • on_complete
Member

prologic commented Sep 4, 2015

Perhaps while we're at it; should we consolidate the .success, .complete and .failure attributes too? (NB:I There's a bug with the complete behavior).

Meaning we break API (for circuits 3.3):

  • on_error
  • on_success
  • on_complete
@spaceone

This comment has been minimized.

Show comment
Hide comment
@spaceone

spaceone Sep 4, 2015

Contributor

Can you give me an example what value "on_success" could have?
(I don't like to work with strings as it is error prone).

Contributor

spaceone commented Sep 4, 2015

Can you give me an example what value "on_success" could have?
(I don't like to work with strings as it is error prone).

@prologic

This comment has been minimized.

Show comment
Hide comment
@prologic

prologic Sep 4, 2015

Member

Good question; perhaps:

e = Event()
e.on_success = "event_to_be_fired_on_auccess", "optional_channel"
Member

prologic commented Sep 4, 2015

Good question; perhaps:

e = Event()
e.on_success = "event_to_be_fired_on_auccess", "optional_channel"
@spaceone

This comment has been minimized.

Show comment
Hide comment
@spaceone

spaceone Sep 4, 2015

Contributor

What do you think about something more encoupled and OOP like the following:

class EventOnNothing(object):
»   def task(self, manager, state):
»   »   pass

class OnSuccess(EventOnNothing):
»   def __init__(self, event_name, *channels):
»   »   self.event_name = event_name
»   »   self.channels = channels

»   def task(self, manager, state):
»   »   if self.event_name:
»   »   »   manager.fire(self.event_name, *self.channels)


class OnError(EventOnNothing):
»   def __init__(self, abort=False):
»   »   self.abort = abort

»   def task(self, manager, state):
»   »   if self.abort:
»   »   »   raise StopIteration


class Event(object):  # the circuits.Event class
»   def __init__(self):
»   »   self.on_error = EventOnNothing()
»   »   self.on_success = EventOnNothing()
»   »   self.on_complete = EventOnNothing()
Contributor

spaceone commented Sep 4, 2015

What do you think about something more encoupled and OOP like the following:

class EventOnNothing(object):
»   def task(self, manager, state):
»   »   pass

class OnSuccess(EventOnNothing):
»   def __init__(self, event_name, *channels):
»   »   self.event_name = event_name
»   »   self.channels = channels

»   def task(self, manager, state):
»   »   if self.event_name:
»   »   »   manager.fire(self.event_name, *self.channels)


class OnError(EventOnNothing):
»   def __init__(self, abort=False):
»   »   self.abort = abort

»   def task(self, manager, state):
»   »   if self.abort:
»   »   »   raise StopIteration


class Event(object):  # the circuits.Event class
»   def __init__(self):
»   »   self.on_error = EventOnNothing()
»   »   self.on_success = EventOnNothing()
»   »   self.on_complete = EventOnNothing()
@spaceone

This comment has been minimized.

Show comment
Hide comment
@spaceone

spaceone Sep 4, 2015

Contributor

Maybe we could outsource behavior in Manager.processTask into some kind of plugins or something. (The method is marked as too complex).

Contributor

spaceone commented Sep 4, 2015

Maybe we could outsource behavior in Manager.processTask into some kind of plugins or something. (The method is marked as too complex).

@prologic

This comment has been minimized.

Show comment
Hide comment
@prologic

prologic Sep 4, 2015

Member

Hmm I think what you've presented adds a lot of complexity too as well as overhead for the developer :) I'm not really a big fan of OOP in general; that's why "components" are my preferred pattern! I agree that we should refactor out some of this stuff; specifically I think coroutines should be factored out somehow as well.

Member

prologic commented Sep 4, 2015

Hmm I think what you've presented adds a lot of complexity too as well as overhead for the developer :) I'm not really a big fan of OOP in general; that's why "components" are my preferred pattern! I agree that we should refactor out some of this stuff; specifically I think coroutines should be factored out somehow as well.

@spaceone spaceone added the enhancement label Dec 4, 2015

prologic added a commit that referenced this issue Jan 31, 2017

@prologic prologic self-assigned this Jan 31, 2017

@prologic

This comment has been minimized.

Show comment
Hide comment
@prologic

prologic Jan 31, 2017

Member

How about #220 ?

Member

prologic commented Jan 31, 2017

How about #220 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment