Skip to content

logic functions#36

Open
grunkgrunk wants to merge 1 commit intorxi:masterfrom
grunkgrunk:master
Open

logic functions#36
grunkgrunk wants to merge 1 commit intorxi:masterfrom
grunkgrunk:master

Conversation

@grunkgrunk
Copy link

Hey! Thanks for creating this library, it's awesome!

I have added some "functional" versions of the logical operations and, or and not. These can for example be used together with lume.filter. To get all enemies with low health that have just been damaged we might call

lume.filter(actors, lume.And(hasLowHealth, isEnemy,  wasJustDamaged)  )

I realize the capital letter in And is a bit at odds with the naming convention of the library. However I was not allowed to name them in all lowercase letters (which makes sense I guess). If you have any suggestions about naming or anything else I'm happy to change things.

If these functions end up being part of the library I'll of course write some documentation as well. If not then that's of course totally fair :))

Cheers!

@jaythomas
Copy link
Contributor

Maybe just my personal taste, but if this is purely for aesthetics I find using lume.all and lume.any to be easier to read:

-- All of these
lume.all({ hasLowHealth, isEnemy,  wasJustDamaged })

-- Any of these
lume.any({ hasLowHealth, isEnemy,  wasJustDamaged })

@grunkgrunk
Copy link
Author

grunkgrunk commented Aug 9, 2021

Right, but hasLowHealth and the others are functions. Sorry I forgot to mention that. So I don't think your approach would do the same thing? What lume.And does is that it creates a new function - in this case it creates the function f(x) = hasLowHealth(x) and isEnemy(x) and wasJustDamaged(x).

@jaythomas
Copy link
Contributor

jaythomas commented Aug 10, 2021

No sorry that's my fault as I didn't read the diff all the way through before commenting. Your approach makes perfect sense then for your scenario since you're providing those predicate functions to lume.all/lume.any to build these higher order functions.

On a deeper level, I don't know what @rxi's criteria is for accepting new functions to this library. Being this library and your change is tiny there is little worry about bloat, but I can also see the argument in perhaps splitting lume's monolith up into separate rocks ala lodash where you can either require the entire library (require('lodash')) or cherry-pick just the functions you want (require('lodash.reduce')) to cut down on application size and memory footprint. That would allow features like yours to be accepted into the fold with less worry about bloat.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants