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

feat: add .inspect() and .inspect_err() methods #185

Merged
merged 1 commit into from
Jun 2, 2024

Conversation

deliro
Copy link
Contributor

@deliro deliro commented May 29, 2024

Hello! In Rust 1.76 inspect and inspect_err methods were stabilized what are very handy when an error (or a value) must be logged. So I implemented them here.

Instead of doing this:

def get_x_from_somewhere() -> Result[..., str]: ...


x = get_x_from_somewhere()
match x:
    case Err(e):
        logger.error("hello world %s", e)

y = x.and_then(upload_to_s3_bucket)

You can now do that:

def get_x_from_somewhere() -> Result[..., str]: ...


y = get_x_from_somewhere().inspect_err(lambda e: logger.error("hello world %s", e)).and_then(upload_to_s3_bucket)

@LEVLLN
Copy link

LEVLLN commented May 29, 2024

+1

@francium
Copy link
Member

Good suggestion. Will review later today

Copy link
Member

@francium francium left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Just a few small changes are needed and we can get this merged.

CHANGELOG.md Outdated
@@ -13,6 +13,8 @@ Possible log types:

## [Unreleased]

- `[added]` Add `inspect()` and `inspect_err()` methods
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `[added]` Add `inspect()` and `inspect_err()` methods
- `[added]` Add `inspect()` and `inspect_err()` methods (#185)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@@ -1,6 +1,6 @@
from __future__ import annotations

from typing import Callable
from typing import Callable, List
Copy link
Member

@francium francium May 30, 2024

Choose a reason for hiding this comment

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

With the from __future__ import annotations already at line 1, you should be able to use list[...] directly and no need to import List. I'm not 100% sure, but try to make the change and see if mypy and tests pass on GitHub.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if 3.8 has already added this :)
Done

@@ -202,6 +202,19 @@ def or_else(self, op: object) -> Ok[T]:
"""
return self

def inspect(self, op: Callable[[T], None]) -> Result[T, E]:
Copy link
Member

Choose a reason for hiding this comment

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

Change all of the op types from returning None to Any, because we shouldn't unnecessarily restrict the user from using a functions that can also return values, we can just ignore any returned value.

Suggested change
def inspect(self, op: Callable[[T], None]) -> Result[T, E]:
def inspect(self, op: Callable[[T], Any]) -> Result[T, E]:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, nice take!

@@ -213,6 +213,26 @@ def test_and_then() -> None:
assert Err(3).and_then(sq_lambda).and_then(sq_lambda).err() == 3


def test_inspect() -> None:
oks: List[int] = []
assert Ok(2).inspect(lambda x: oks.append(x)) == Ok(2)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add lines to test using a regular function as well, you'll discover the issue of None vs Any as I mentioned in my earlier comment,

# Move to bottom of this file along side the other similar functions
def concat_str(x: str) -> str: 
    return x + x

def test_inspect() -> None:
    Err("e").inspect_err(concat_str)
    Err("e").inspect(concat_str)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a test with a regular function

@@ -213,6 +213,26 @@ def test_and_then() -> None:
assert Err(3).and_then(sq_lambda).and_then(sq_lambda).err() == 3


def test_inspect() -> None:
oks: List[int] = []
assert Ok(2).inspect(lambda x: oks.append(x)) == Ok(2)
Copy link
Member

Choose a reason for hiding this comment

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

Can you define these lambda's similar to the sq_lambda that is defined at the bottom of this file. Just because it's not possible to annotate the x here nicely and my editor gives me poor type checking here currently

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can't be moved to the bottom of the file since every fn is a closure there. We can't test a pure function, because a pure function in inspect* methods is no-op.
But I type-hinted them

Copy link
Member

Choose a reason for hiding this comment

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

You're right. My mistake

@francium francium changed the title add .inspect() and .inspect_err() methods feat: add .inspect() and .inspect_err() methods May 30, 2024
Copy link
Member

@francium francium left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Looks great.

@francium francium merged commit 08a6030 into rustedpy:main Jun 2, 2024
5 checks passed
@francium
Copy link
Member

francium commented Jun 2, 2024

Version 0.17.0 published with this included.

@deliro deliro deleted the inspect-methods branch June 13, 2024 14:36
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.

3 participants