vickumar1981 / pyeffects

Handle side-effects in Python like a boss. Implements functional types for Either, Option, Try, and Future.
Other
30 stars 6 forks source link

Add `on_success` method #19

Closed sloboegen closed 1 year ago

sloboegen commented 1 year ago

Closes #1

Hello! First, thanks for the interesting project :) I would like to contribute a little.

Some points:

vickumar1981 commented 1 year ago

@sloboegen thanks for this! will take a look this weekend and get it merged in.

vickumar1981 commented 1 year ago

@sloboegen

I know you didn't create this bug, and I did, but would you mind changing the name of the class to TestFuture in the test_future.py file?

https://github.com/sloboegen/pyeffects/blob/future-on-success/tests/test_future.py#L7

Thanks

vickumar1981 commented 1 year ago

@sloboegen , it looks like you bumped the python version in the tests, the tox.ini file to 3.9.

https://github.com/sloboegen/pyeffects/blob/future-on-success/tox.ini#L2

B/c this is a library, I didn't want to jump all the way to 3.11 and instead keep it at a minimum python version that could still be used for versions higher up.

Can we bump this to 3.8 instead?

There's also two other places to make a change for the version, in the Pipfile:

https://github.com/vickumar1981/pyeffects/blob/master/Pipfile#L26

And in the setup.py file:

https://github.com/sloboegen/pyeffects/blob/future-on-success/setup.py#L72

Basically, in all three places, let's just go with 3.8 and higher as the standard.

vickumar1981 commented 1 year ago

LGTM. @sloboegen We can either address the comments in this PR or merge this, and open another PR to address the comments.

Your choice.

Thanks again. :+1:

sloboegen commented 1 year ago

Thanks for your feedback. I bumped Python version to 3.8 in three places you have mentioned and added a test for unhappy case