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

feat(collections): add firstNotNullishOf #1167

Merged
merged 2 commits into from
Aug 31, 2021
Merged

feat(collections): add firstNotNullishOf #1167

merged 2 commits into from
Aug 31, 2021

Conversation

majidsajadi
Copy link
Contributor

This PR adds firstNotNullishOf to the collection module.

Copy link
Contributor

@LionC LionC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation can be a bit shorter and I think we should work on the documentation sentence in the README / JSDoc. Rest looks good, but we should clean that up first.

// Copyright 2018-2021 the Deno authors. All rights reserved. MIT license.

/**
* Returns the first value in the given collection that produces a not-nullish value using the given selector
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could simplify that language a bit maybe, the current one is a bit convoluted

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LionC I copied the description from your proposal PR. how do you feed about Kotlin's description with a little bit of change?

Returns the first value that is neither null nor undefined produced by transform function being applied to elements of this array in iteration order, or undefined if no value was found.****

Copy link
Contributor

@LionC LionC Aug 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I noted in the proposal, the descriptions were only for the sake of internal discussion, not for actual documentation. That has been overlooked a lot, but we have to make sure it is as understandable as possible.

Maybe something like this?

Applies the given selector to elements in the given array until a value is produced that is
neither `null` nor `undefined` and returns that value.

Returns `undefined` if no such value is produced.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good to me.

Comment on lines 26 to 37
let result: NonNullable<O> | undefined = undefined;

for (const current of collection) {
const selected = selector(current);

if (selected !== null && selected !== undefined) {
result = selected as NonNullable<O>;
break;
}
}

return result;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can actually remove the result variable completeley:

Suggested change
let result: NonNullable<O> | undefined = undefined;
for (const current of collection) {
const selected = selector(current);
if (selected !== null && selected !== undefined) {
result = selected as NonNullable<O>;
break;
}
}
return result;
for (const current of collection) {
const selected = selector(current);
if (selected !== null && selected !== undefined) {
return selected as NonNullable<O>;
}
}
return undefined;

* ```
*/
export function firstNotNullishOf<T, O>(
collection: T[],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh and we settled on array and readonly for these arguments, we should be ocnsistent here

Suggested change
collection: T[],
readonly array: T[],

@majidsajadi
Copy link
Contributor Author

@LionC Updated.

@LionC LionC mentioned this pull request Aug 27, 2021
44 tasks
Copy link
Contributor

@LionC LionC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, pinging @kt3k

### firstNotNullishOf

Applies the given selector to elements in the given array until a value is
produced that is neither `null` nor `undefined` and returns that value Returns
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a period is missing?

Suggested change
produced that is neither `null` nor `undefined` and returns that value Returns
produced that is neither `null` nor `undefined` and returns that value. Returns

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@majidsajadi Thank you for the work! LGTM

@kt3k kt3k merged commit cbfb04e into denoland:main Aug 31, 2021
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.

None yet

3 participants