search

ramda / ramda-fantasy

:ram::sparkles: Fantasy-Land compatible types for easy integration with Ramda.js
MIT License
1.5k stars 95 forks source link

Added toMaybe static constructor. #133

Closed martinlechner1 closed 7 years ago

martinlechner1 commented 7 years ago

As discussed in Issue #132, I added a proposal for a ofNullable static constructor.

davidchambers commented 7 years ago

This function does not actually take a value of type Nullable a as an argument, since it also treats undefined specially. It's misleading to use nullable in this function's name. I suggest toMaybe, to match S.toMaybe.

I've been using a? to represent the type whose members are null and undefined in addition to all the members of a.

martinlechner1 commented 7 years ago

Thanks for the input. I will change the code accordingly.

On Tue, Nov 8, 2016, 11:57 David Chambers notifications@github.com wrote:

This function does not actually take a value of type Nullable a https://github.com/sanctuary-js/sanctuary-def#nullable as an argument, since it also treats undefined specially. It's misleading to use nullable in this function's name. I suggest toMaybe, to match S.toMaybe https://sanctuary.js.org/#toMaybe.

I've been using a? to represent the type whose members are null and undefined in addition to all the members of a.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ramda/ramda-fantasy/pull/133#issuecomment-259107207, or mute the thread https://github.com/notifications/unsubscribe-auth/AGXLaZ6RT8oA2fobkRLWtx6uO9Yn9F7Oks5q8FWYgaJpZM4KruON .

davidchambers commented 7 years ago

The changes look good. Could you update the pull request's title accordingly?

TheLudd commented 7 years ago

This is essentially an alias of the constructor. What about ramdas no alias policy?

martinlechner1 commented 7 years ago

The primary motivation was to not use the constructor directly to have a cleaner general code style while using the lib. See: http://eslint.org/docs/rules/new-cap Having Maybe.toMaybe(value) is in my opinion superior to Maybe(value), since you express your intent more cleanly.

TheLudd commented 7 years ago

I disagree with the claim that toMaybe is more clear. That name does not reveal anything except that it returns a Maybe.

Regarding the rule, I have been forced to deactivate it on some projects since there are functions that are not constructors but that do start with a capital letter such as the I, K combinators.

CrossEye commented 7 years ago

One more reason I don't like the single-letter combinator names! :smile:

TheLudd commented 7 years ago

One more reason I don't like the single-letter combinator names!

Haha to each his own but I prefer eslint to be a helper and not rule my world =)

CrossEye commented 7 years ago

No, I turn off linting rules all the time. I've enforced similar rules in JS codebases long before I had linters available; it has seemed a good convention for JS.

My main objection to the single-letter names is how nondescriptive they are. I have the same problem using bird names. I like for me and others to be able to read the code!

martinlechner1 commented 7 years ago

What about the merge request? There is another not mentioned benefit. The classic constructor does not handle the undefined case (which I would like to have handled too).

TheLudd commented 7 years ago

The classic constructor does not handle the undefined case

yes it does. x == null equals true for undefined and null

scott-christopher commented 7 years ago

Closing as implemented via #159.