Open dimitri-xyz opened 7 years ago
Sorry for the late reply.
• Changing actuate
to activate
sounds fine to me. (I originally chose the word actuate
because it has a slightly different connotation that matches better with pause
.)
• Concerning AddHandler
, I agree that a noun is better. However, the trouble is that in Haskell, I would expect that the noun HandlerSet
represents a pure set, essentially meaning type HandlerSet = Set Handler
in some way or another. Unfortunately, the set of handlers contained within an AddHandler
cannot be manipulated with pure Haskell functions, but only with the somewhat arcane imperative addHandler
function. Thus, the name HandlerSet
would be misleading. The name AddHandler
is more appropriate.
The name HandlerManager
would work as well, though I did like that the name matched the name of its sole function, addHandler
.
That said, I'm also open to suggestions on how to make AddHandler
more pure. The main argument for introducing it in the first place is that some GUI frameworks, in particular GTK, provide addHandler
out of the box, thus reducing the need for glue code.
• Concerning Event
, I agree that EvenStream
is more appropriate, but it is too long. Also, Event
in the sense of "event stream" has been introduced by Conal Elliott, so there are historical reasons for keeping it.
That said, using the plural, Events
, would be a neat way to solve this problem. But this is a big change that I would like to poll library users about, first.
I really like your idea of using the plural Events
! It is short and much clearer.
Here's a follow on thought for your consideration. When learning to use reactive-banana (I still am), I did not understand why we had to go through the extra step of grouping the handlers together to get an Event
inside the EventNetwork
. It seemed like an unnecessary step. Instead of the default setup being.
main = do
-- newAddHandler :: IO (AddHandler a, Handler a)
(myHandlers, fire) <- newAddHandler
-- describe the event network
let networkDescription :: MomentIO ()
networkDescription = mdo
-- fromAddHandler :: AddHandler a -> MomentIO (Event a)
myEvent <- fromAddHandler myHandlers
...
fire someValue
In other words, the Event
s get into the EventNetwork
though the AddHandler
. It seemed an easier setup would be:
main = do
-- newHandler :: IO (Handler a)
handler <- newHandler
-- describe the event network
let networkDescription :: MomentIO ()
networkDescription = mdo
-- fromHandler :: Handler a -> MomentIO (Event a)
myEvent <- fromHandler handler
...
fire handler someValue
In other words, it seems the "grouping" should be optional not mandatory. I seldom register
multiple callbacks with the same AddHandler
to be able to fire more than one "simultaneously". So, your explanation above for the reason for its introduction made a lot of sense. Anyway, just an extra thought.
Here is another suggestion: change the type of reactimate
to:
reactimate :: Event a -> (a -> IO ()) -> MomentIO ()
in order to facilitate the very common case of working with some non-IO ()
event in the actual network logic, and only fmap
ping over events at the very end to reactimate. Basically, it would allow rewriting
reactimate $
(\x -> do
my_big_action
lots_of_lines
print x)
<$> myEvent
as
reactimate myEvent $ \x -> do
my_big_action
lots_of_lines
print x
You can already do:
reactimate $ ev <&> \x -> do ...
That's true, and my suggestion is fairly annoying for <$
usage, isn't it... Where does <&>
come from these days? Lens?
Data.Functor
has $>
for that. blah $> do ...
.
<&>
I thought was in base
but apparently is not.
I am:
actuate
to activate
. I'm a native English speaker and I don't even know what actuate means, but the API for working with EventNetwork
s is so tiny that I don't think there's much chance of confusion.Event
to Events
AddHandler
, because the name is so difficult for me to understand. I unfortunately don't like HandlerSet
much. Handlers
?Oh, and I'm -1 on the Handler
type synonym. I don't like type synonyms much in general :)
I propose to change unionWith
and mergeWith
(not yet merged) to union
and merge
My reasoning is:
Neither name is imported by Prelude.
In unionWith, it's obvious (to me) what the given function does. I'd be similarly unhappy if filter was instead called filterWith. Of course it filters with a predicate because it's an argument to the function.
I know that a previous version of reactive-banana exported both union and unionWith, but since union is gone, we should reclaim its nice, short name :)
For mergeWith, the "with" is awkwardly referring to three separate arguments.
To avoid a major version bump (if that's a concern) I'd also be totally happy with leaving unionWith around, but deprecated in favor of union.
Or, if you prefer the names unionWith and mergeWith, that's fine too :)
@HeinrichApfelmus says:
The main reasoning behind the names was that they be consistent with the names in the Data.Map module. But there may be better arguments in favor of the new names.
I think the name of some of the functions and types used in Reactive Banana could be improved to help beginners and also to more closely match the semantic meaning of the underlying operations and types. Here are a few suggestions that I think are important.
Change
actuate
toactivate
— These words have very similar meanings and I believe are interchangeable in the context ofEventNetwork
s. However, activate is much more common. A cursory google search has 246 million hits for activate against only 6.7 million for actuate. So, activate is over 30 times more common. This makes it easier for non-native English speakers to know its meaning.Change
AddHandler
toHandlerSet
— I think it is easier to have a type denote nouns (such as a network connection, a list or a hashMap) as opposed to verbs. Those are usually associated with functions (such as in filter, interpret and apply). In the case ofAddHandler
, “Add” is a verb.Semantically, this types provides a Set of event handlers that are all “fired” together. There is no internal structure, so this is not a list. A HandlerRegistrar or similar name would also work, but the word registrar is not used much. Derived functions such as “newAddHandler” would also have clearer names (e.g. newAddHandler becomes newHandlerSet).
Change
Event
toEventStream
— I understand this may be a more controversial change. I believe the nameEvent
has stuck for historical reasons. However, I believe it is very confusing in the context of Reactive Banana. In common day usage, an event happens only once, whereas in Reactive Banana the typeEvent a
encompasses multiple events that happen at different times. From the docs “Event a
represents a stream of events as they occur in time.” This leads to some confusion. The new name would more closely reflect the type’s semantic meaning. Note that there is no such problem with Behaviors.I think it is easy to make these changes in the code and documentation, but I wouldn't want to make a pull request before the discussion. To keep backwards compatibility, it is only necessary to make name aliases. For example:
I think Haskell’s Achilles’ heel is its barrier to entry. The intent of this suggestion is to make Reactive Banana easier to learn and understand.