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

refactor: improve groupBy #2532

Merged
merged 7 commits into from
Nov 23, 2023
Merged

refactor: improve groupBy #2532

merged 7 commits into from
Nov 23, 2023

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Nov 8, 2023

Adds a value mapping function to groupBy.

Intended for #2530

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs labels Nov 8, 2023
@ST-DDT ST-DDT added this to the vAnytime milestone Nov 8, 2023
@ST-DDT ST-DDT requested review from a team November 8, 2023 10:36
@ST-DDT ST-DDT self-assigned this Nov 8, 2023
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Merging #2532 (cab1af5) into next (9b00fe9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2532   +/-   ##
=======================================
  Coverage   99.56%   99.57%           
=======================================
  Files        2805     2805           
  Lines      250097   250126   +29     
  Branches     1099     1103    +4     
=======================================
+ Hits       249018   249054   +36     
+ Misses       1051     1044    -7     
  Partials       28       28           
Files Coverage Δ
src/internal/group-by.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Copy link
Member

@xDivisionByZerox xDivisionByZerox left a comment

Choose a reason for hiding this comment

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

I feel like this missed the point of "grouping" by a key. This function would then break the single responsibility principle.

Note that the following polifills (which this function was inspired by) do not include a value mapper as well:

@ST-DDT
Copy link
Member Author

ST-DDT commented Nov 9, 2023

In the Java world they noticed quite early (Java 8), that there is a need to give the user the ability to also process/transform the value.
Common usecases include wrapper shedding, which is the reason why I added the parameter in the first place.

groupBy(
  [
    [1, "someValue"],
    [2, "blub"],
    [1, "anotherone"]
  ], 
  (e) => e[0],
  (e) => e[1]
);

=>

{
  1: ["someValue", "anotherone"],
  2: ["blub"]
}

I can explain this in more detail in the next team-meeting if you would like to.

@xDivisionByZerox
Copy link
Member

xDivisionByZerox commented Nov 14, 2023

Please add the behaviour to the indended place as #2530 is already merged. I'm fine with keeping the additional utility.

@ST-DDT ST-DDT requested review from xDivisionByZerox and a team November 15, 2023 09:38
@xDivisionByZerox
Copy link
Member

"Off-topic" question: Should we have tests for our internal function?

@ST-DDT ST-DDT requested review from xDivisionByZerox and a team November 15, 2023 23:13
@xDivisionByZerox xDivisionByZerox enabled auto-merge (squash) November 23, 2023 19:04
@xDivisionByZerox xDivisionByZerox merged commit c2b9523 into next Nov 23, 2023
20 checks passed
@ST-DDT ST-DDT deleted the refactor/internal/groupBy branch February 5, 2024 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants