search

spinettaro / delphi-event-bus

Delphi Event Bus (for short DEB) is an Event Bus framework for Delphi
Apache License 2.0
466 stars 110 forks source link

Dkounal instancecontext #55

Closed dkounal closed 3 years ago

dkounal commented 3 years ago

Add the possibility to define instance based Context: If a property with Context named "InstanceBased" exists, then with the RegisterCustomContextSubscriberForEvents you can change the context per Object instance that uses this type of Interface-message.

wxinix commented 3 years ago

  1. If the same subscriber object is registered twice, one with the old method RegisterSubscriberForEvents, the other with the new RegisterCustomContextSubscriberForEvents, then, UnRegisterForEvents will have problem - because it will remove all registrations, leading to side effects.

  2. The coding style is not consistent with the existing system.

dkounal commented 3 years ago
  1. You do not need to use both register methods. If you use the original one, you do not have the new feature. If you use the new register method, registrations work as usual and only attributes with this custom context changed
  2. I am open to suggestions
  3. Existing tests passed
wxinix commented 3 years ago 
3\. Existing tests passed
  1. You added a new method, which needs new test case.
  2. Generally register and unregiser methods need to be paired.
  3. From a design perspective, "instancebased" itself is a flag, as such, probably it should be moved as an additional boolean field of the Attribute class, not as a candidate value for the Context string which might be a source of confusion because of the mixed concerns.

You had a good start. I am just quibbling.

spinettaro commented 3 years ago

I agree with @wxinix points. Most important one is unit tests. I'm going to close it and move on #57 @dkounal do you mind ?

dkounal commented 3 years ago

It is OK