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

Support key exclusions #9

Closed
Richienb opened this issue Oct 26, 2020 · 14 comments · Fixed by #18
Closed

Support key exclusions #9

Richienb opened this issue Oct 26, 2020 · 14 comments · Fixed by #18

Comments

@Richienb
Copy link
Contributor

Something like:

const filterObject = require('filter-obj');

filterObject({a: 1, b: 1, c: 1} {keys: ['!a']});
//=> {b: 1, c: 1}

Source: sindresorhus/query-string#282 (comment)

@sindresorhus
Copy link
Owner

Makes sense 👍🏻

@faizanu94
Copy link

@Richienb this feature is available through below function being passed as predicate, also has a flexibility for keys as well as values:

filterObject({a: 1, b: 1, c: 1}, (key, value) => key !== 'a');

@Richienb
Copy link
Contributor Author

Richienb commented Nov 4, 2020

@faizanu94 This will be used for sindresorhus/query-string#282 (comment)

@faizanu94
Copy link

I would say then exclusions should be available for values as well, @sindresorhus your thoughts on this?

@sindresorhus
Copy link
Owner

@faizanu94 I don't think that is as common of a need. If someone needs that, they can propose it then or just use the filter. I'm mostly just interested in the API sugar for keys.

@ehmicky
Copy link
Contributor

ehmicky commented Jun 3, 2022

Please correct if I'm wrong, but I believe, since we are using string matching and not globbing patterns, it would not make sense to mix excluded keys with included keys, such as:

filterObject({a: 1, b: 1, c: 1}, ['!a', 'b']);

Since it would have the same behavior as passing only ['b'].

Based on this, if a user wants to exclude keys, they would probably want to prepend ! to all keys, which might be slightly tedious:

filterObject({a: 1, b: 1, c: 1}, ['!a', '!b', '!c', '!d', '!e', '!f']);

Also, behavior might be ambiguous when both including and excluding the same key. Either should have priority, which would need to be documented and might create some confusion among some users.

filterObject({a: 1, b: 1, c: 1}, ['!a', 'a']);

I am wondering whether it would be simpler instead to export a separate function negating the predicate? It would also be more performant since the logic would need to do fewer argument checks and could be optimized for each case. Finally, it would also work with predicate functions.

import { include, exclude } from 'filter-obj';

include({a: 1, b: 2, c: 3}, ['a']); // {a: 1}
exclude({a: 1, b: 2, c: 3}, ['a']); // {b: 2, c: 3}

include({a: 1, b: 2, c: 3}, (key, value) => value === 1); // {a: 1}
exclude({a: 1, b: 2, c: 3}, (key, value) => value === 1); // {b: 2, c: 3}

@Richienb
Copy link
Contributor Author

Richienb commented Jun 3, 2022

Yes, that is a problem I immediately stumbled upon as soon as I began to think of how to implement it. If you're suggesting to create separate include and exclude functions then logically, the original function should lose that functionality for simplicity. The new API could be something like this:

  • excludeObjectKeys
  • filterObjectKeys to communicate that it is the most common use, includeObjectKeys to be more explicit and to show that it doesn't filter values or pickObjectKeys for a more TypeScript approach that is also analogous to query-string
  • filterObject to be short and sweet or filterObjectPredicate to be explicit

@ehmicky
Copy link
Contributor

ehmicky commented Jun 4, 2022

I was actually going to suggest this too! Meaning: breaking it into three methods instead: two with array of keys (with/without negation) and one with the function predicate.

When it comes to the names, I like them too, with two additional questions:

  • Should ObjectKeys be shortened to just Keys? Since keys are usually understood to be object keys in JavaScript.
  • I like pick*() too as it is what Lodash and some other functional libraries use. However, the other method should probably be omit*() instead of exclude*() then, since those are more "contrary". Thoughts?

@Richienb
Copy link
Contributor Author

Richienb commented Jun 4, 2022

Should ObjectKeys be shortened to just Keys?

The Object part is more about keeping the naming similar to the predicate function though I suppose this more pragmatic approach would be fine.

the other method should probably be omit*()

Sure, though I'm not sure whether the names need to conform with other Sindre modules.

@sindresorhus What do you think?

@sindresorhus
Copy link
Owner

I prefer includeKeys and excludeKeys.

Richienb added a commit to Richienb/filter-obj that referenced this issue Jul 21, 2022
…eKeys()`

Fixes sindresorhus#9

Signed-off-by: Richie Bendall <richiebendall@gmail.com>
@ehmicky
Copy link
Contributor

ehmicky commented Jul 21, 2022

👍 To summarize it, this would be:

import { filterObject, includeKeys, excludeKeys } from 'filter-obj';

filterObject({a: 1, b: 2, c: 3}, (key, value) => value === 1); // {a: 1}
filterObject({a: 1, b: 2, c: 3}, ['a']); // Error. This is a breaking change.
includeKeys({a: 1, b: 2, c: 3}, ['a']); // {a: 1}
excludeKeys({a: 1, b: 2, c: 3}, ['a']); // {b: 2, c: 3}

Is this correct?

@Richienb
Copy link
Contributor Author

I merged filterObject and includeKeys into a single function:

import {includeKeys, excludeKeys} from 'filter-obj';

const object = {
	foo: true,
	bar: false
};

const newObject = includeKeys(object, (key, value) => value === true);
//=> {foo: true}

const newObject2 = includeKeys(object, ['bar']);
//=> {bar: false}

const newObject = excludeKeys(object, (key, value) => value === true);
//=> {bar: false}

const newObject3 = excludeKeys(object, ['bar']);
//=> {foo: true}

@ehmicky
Copy link
Contributor

ehmicky commented Jul 21, 2022

This works. 👍

This would also be close to this stage 1 ES proposal for Object.pick|omit(), which would be very similar to what filter-obj would then be doing.

@sindresorhus What are your thoughts?

@ehmicky
Copy link
Contributor

ehmicky commented Jul 23, 2022

Thanks @Richienb for adding this feature! ❤️

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

Successfully merging a pull request may close this issue.

4 participants