Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Name change suggestions #147

Open
dimitri-xyz opened this issue Mar 13, 2017 · 9 comments
Open

Name change suggestions #147

dimitri-xyz opened this issue Mar 13, 2017 · 9 comments

Comments

@dimitri-xyz
Copy link

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 to activate — These words have very similar meanings and I believe are interchangeable in the context of EventNetworks. 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 to HandlerSet — 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 of AddHandler, “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 to EventStream — I understand this may be a more controversial change. I believe the name Event 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 type Event 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:

type AddHandler = HandlerSet
newAddHandler   = newHandlerSet
fromAddHandler  = fromHandlerSet
actuate         = activate

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.

@HeinrichApfelmus
Copy link
Owner

HeinrichApfelmus commented Apr 29, 2017

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.

@dimitri-xyz
Copy link
Author

dimitri-xyz commented Apr 29, 2017

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 Events 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.

@mitchellwrosen
Copy link
Collaborator

mitchellwrosen commented Sep 7, 2017

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 fmapping 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

@ocharles
Copy link
Collaborator

ocharles commented Sep 8, 2017

You can already do:

reactimate $ ev <&> \x -> do ...

@mitchellwrosen
Copy link
Collaborator

mitchellwrosen commented Sep 8, 2017

That's true, and my suggestion is fairly annoying for <$ usage, isn't it... Where does <&> come from these days? Lens?

@ocharles
Copy link
Collaborator

ocharles commented Sep 8, 2017

Data.Functor has $> for that. blah $> do ....

<&> I thought was in base but apparently is not.

@mitchellwrosen
Copy link
Collaborator

I am:

  • -1 on changing actuate to activate. I'm a native English speaker and I don't even know what actuate means, but the API for working with EventNetworks is so tiny that I don't think there's much chance of confusion.
  • +1 on changing Event to Events
  • +1 on changing the name of AddHandler, because the name is so difficult for me to understand. I unfortunately don't like HandlerSet much. Handlers?

@mitchellwrosen
Copy link
Collaborator

mitchellwrosen commented May 22, 2018

Oh, and I'm -1 on the Handler type synonym. I don't like type synonyms much in general :)

@mitchellwrosen
Copy link
Collaborator

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.

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

No branches or pull requests

4 participants