Skip to content

Conversation

@samer-hamood
Copy link
Contributor

@samer-hamood samer-hamood commented Jan 30, 2025

This PR adds the zip_with_next function, which zips together adjacent elements, or seq[0:-1] with seq[1:].

Its functionality is common (think of how many algorithms require processing adjacent elements together), therefore it makes sense to include it in Pyfunctional's API and not leave it to clients to add it as an @extend function.

Kotlin's collections API already has this function, and adding it to Pyfunctional makes it even more serviceable and versatile.

@samer-hamood
Copy link
Contributor Author

samer-hamood commented Feb 7, 2025

Hello - My apologies, but this PR has been open for several weeks and I can't move forward until after this PR Is processed.

@EntilZha
Copy link
Owner

Thanks, I'll review it later today when I get some time.

@samer-hamood
Copy link
Contributor Author

Thanks, I'll review it later today when I get some time.

Thank you

@samer-hamood
Copy link
Contributor Author

samer-hamood commented Feb 23, 2025

Hi @EntilZha , let me know if you are still maintaining this project. I ask because you have yet to approve/comment on this PR after 3 or more weeks.

It's okay if you don't want people to create PRs for this project due to not having enough free time or whatever the reason, but let us know so we can use our time more constructively on other projects.

Thanks

@EntilZha
Copy link
Owner

Hi @samer-hamood, I still maintain the project, but my time is often pretty bursty as I have free time.

Thanks for the Kotlin example, thats enough to convince me its worth adding. I'll do a quick review of code and leave any comments and/or approve.


[tool.poetry.group.dev.dependencies]
black = "^23.1"
parametrize = "^0.1.1"
Copy link
Owner

Choose a reason for hiding this comment

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

It looks like pytest has this natively https://docs.pytest.org/en/stable/how-to/parametrize.html, so I'd prefer using that and it doesnt look like parametrize has been updated since 2021.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is that pytest parametrize does not work with unittest.TestCase which your tests are using, that's why the library was added; has that changed?

Copy link
Owner

Choose a reason for hiding this comment

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

Not that I know, I just didn't realize it didn't work.

self.assertIteratorEqual(result, e)
self.assert_type(result)

@parametrize(
Copy link
Owner

Choose a reason for hiding this comment

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

Using https://docs.pytest.org/en/stable/how-to/parametrize.html would avoid adding another library

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It would also seemingly mean I would have to move the test out of the class or change it somehow to fit pytest parameterize, which would be more work, possibly refactoring the tests in that file.

I was hoping to avoid this by adding the library; it wasn't something I wanted to do.

Copy link
Owner

Choose a reason for hiding this comment

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

Fair enough, I didn't realize until reading about the library that you couldn't use pytest natively with classes.

Copy link
Owner

@EntilZha EntilZha left a comment

Choose a reason for hiding this comment

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

Overall, LGTM. If you can swap the parametrize from the standalone library to the pytest version https://docs.pytest.org/en/stable/how-to/parametrize.html, I'm happy to merge

@EntilZha EntilZha merged commit 78bc7c2 into EntilZha:master Mar 13, 2025
5 checks passed
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.

2 participants