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: Update Window operation in point free style. #179

Merged
merged 7 commits into from
Aug 22, 2021

Conversation

drupol
Copy link
Collaborator

@drupol drupol commented Aug 15, 2021

This PR:

  • Update Window in point free style.
  • Has unit tests (phpspec)
  • Has updated tests
  • Has updated documentation
  • Is a minor BC break - The behavior has changed when the $size is zero, which is more in sync with what the Window operation does and avoid mixing types (T|list<T> => list<T>)
    Before: Collection::fromIterable(['a', 'b', 'c'])->window(0); // ['a', 'b', 'c']
    After: Collection::fromIterable(['a', 'b', 'c'])->window(0); // [['a'], ['b'], ['c']]

@drupol drupol added the enhancement New feature or request label Aug 15, 2021
@drupol drupol changed the title refactor: Update Window operation in point free style. refactor: Update Window operation in point free style. Aug 15, 2021
@drupol drupol marked this pull request as ready for review August 16, 2021 20:14
@drupol drupol enabled auto-merge (squash) August 17, 2021 21:21
@drupol drupol marked this pull request as draft August 18, 2021 20:27
auto-merge was automatically disabled August 18, 2021 20:27

Pull request was converted to draft

@drupol drupol self-assigned this Aug 18, 2021
@drupol drupol marked this pull request as ready for review August 19, 2021 13:19
@drupol drupol removed their assignment Aug 19, 2021
@drupol drupol enabled auto-merge (squash) August 19, 2021 17:14
@drupol drupol merged commit e59b29b into master Aug 22, 2021
Comment on lines +4050 to +4052
// Unsupported - but tested.
$this::fromIterable(range('a', 'e'))
->window(-2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about the window(-1) case? That one is mentioned now in the docs so sounds like it's supported -> we should have a test for it

@drupol drupol deleted the refactor/update-window-operation branch August 22, 2021 20:41
@AlexandruGG
Copy link
Collaborator

@drupol I think you merged the wrong one? 😅

@drupol
Copy link
Collaborator Author

drupol commented Aug 22, 2021

Oh. Sorry :( Indeed I messed up... too many tabs open here.

@drupol
Copy link
Collaborator Author

drupol commented Aug 22, 2021

I'll add a new test in the master branch directly in a few minutes.

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

Successfully merging this pull request may close these issues.

2 participants