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

Add NonEmptyBlock class and utility functions #178

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

brendanmaguire
Copy link
Contributor

  • This collection guarentees to have at least one element
  • Uses Scala cats NonEmptyList as inspiration

Closes #177

* This collection guarentees to have at least one element
* Uses Scala cats [NonEmptyList](https://typelevel.org/cats/datatypes/nel.html) as inspiration

Closes dbrattli#177
@brendanmaguire
Copy link
Contributor Author

brendanmaguire commented Nov 11, 2023

This is still a WIP. I have tests running locally but need to fix them up and push. Edit: Done

@dbrattli , interested to hear your thoughts. Would this be a collection type you'd be willing to merge?

* Fix `non_empty_block._validate
* Remove `non_empty_block.unfold` because it is not really needed with `block.unfold` available
* Fix formatting
* Added unit tests
* Removed `_head` and `_tail` as the same functionality is better implemented with `_block`
@brendanmaguire brendanmaguire marked this pull request as ready for review November 13, 2023 17:45
Comment on lines +105 to +116
# TODO: Not sure how to make this pass pyright
# Gives the following type of error:
# Type parameter "_TSource@NonEmptyBlock" is invariant, but "int" is not the same as
# "_TSourceSum@sum | Literal[0]"
#def test_non_empty_block_sum():
# assert _test_value.sum() == 10
# assert _test_value.pipe(non_empty_block.sum) == 10
#
#
#def test_non_empty_block_sum_by():
# assert _test_value.sum_by(lambda i: i * 10) == 100
# assert _test_value.pipe(non_empty_block.sum_by(lambda i: i * 10)) == 100
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why this doesn't work. I looked at test_array.py#test_array_sum_works which is similar and passes the type checker, and I can't see what I'm doing wrong here 🤷

@dbrattli
Copy link
Owner

Looks very nice. Could we perhaps subclass Block to avoid so much code duplication? E.g by using the new Self type in block.py so functions return NonEmptyBlock instead of Block?

@brendanmaguire
Copy link
Contributor Author

Looks very nice. Could we perhaps subclass Block to avoid so much code duplication? E.g by using the new Self type in block.py so functions return NonEmptyBlock instead of Block?

Thanks for taking a look 👍

Some methods were not implemented in NonEmptyBlock as they wouldn't make sense. And some new ones were implemented. This change might involve pulling out the common part to an abstract BaseBlock class.

It might be a while before I get back to this PR again but I will take a look into that and see what's possible at some point.

@brendanmaguire
Copy link
Contributor Author

Hey @dbrattli . I just got around to looking at refactoring this to make NonEmptyBlock a subtype of Block. It seems it won't work because Self cannot be parameterized.

For example, if I change Block#collects signature to:

    def collect(self, mapping: Callable[[_TSource], Block[_TResult]]) -> Self[_TResult]:

I get the following error from pyright:

Expected no type arguments for class "Self"

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.

NonEmptyList
2 participants