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 runningReduce #1226

Merged
merged 4 commits into from
Sep 13, 2021
Merged

feat (collections): add runningReduce #1226

merged 4 commits into from
Sep 13, 2021

Conversation

ayame113
Copy link
Contributor

last part of #1173

  • Did you support importing from mod.ts as well as your functions file?
  • Have you made sure to allocate as little memory and do as little steps in your algorithm as possible?
  • Did you add/adapt JSDoc comments, add/adapt an example, made sure that the example uses the ts markdown tag and assertEquals to show the result?
  • Did you add/adapt your functions documentation in README.md?
  • Did you make sure not to mutate the arguments passe to your function?
  • Did you add tests ensuring no mutation, empty input and corner cases?
  • Are your types flat, meaning there is no unnecessary alias in them that makes your users "go to type definition" twice to understand what to pass?

@LionC LionC mentioned this pull request Sep 12, 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.

Thanks for implementing this! Loos good, I just added naming and API comments for consistency with the module. I really like the implementation.

collections/running_reduce.ts Outdated Show resolved Hide resolved
collections/running_reduce.ts Outdated Show resolved Hide resolved
collections/running_reduce.ts Outdated Show resolved Hide resolved
initialValue: A,
): A[] {
let _initialValue = initialValue;
return collection.map((el) => _initialValue = reducer(_initialValue, el));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually pretty smart!

I would personally put the function body on a newline, as a lot of things are happening here, the line is hard enough to read as it is and it hides a statements in there.

Suggested change
return collection.map((el) => _initialValue = reducer(_initialValue, el));
return collection.map((el) =>
_initialValue = reducer(_initialValue, el)
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I apply this change, it doesn't seem to pass the deno fmt check.

@ayame113
Copy link
Contributor Author

@LionC
Thank you for your review, I have updated the PR.

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

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.

@ayame113 LGTM. Nice work!

@LionC Thank you for reviewing in detail

@kt3k kt3k merged commit d561b7d into denoland:main Sep 13, 2021
@ayame113 ayame113 deleted the feat-collections-runnning-reduce branch September 13, 2021 12:06
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