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

Feature/transform #60

Merged
merged 6 commits into from
May 12, 2023
Merged

Feature/transform #60

merged 6 commits into from
May 12, 2023

Conversation

acostapazo
Copy link
Contributor

No description provided.

meiga/result.py Outdated
@@ -31,6 +37,7 @@ def __init__(
self._value_success = success
self._value_failure = failure
self._assert_values()
self._inner_transformer: Union[Callable[[Result], Any], None] = None
Copy link

Choose a reason for hiding this comment

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

  • maybe use Optional here and in transform(), since it is already imported?
  • * Result[TS, TF]

Copy link
Contributor Author

@acostapazo acostapazo May 12, 2023

Choose a reason for hiding this comment

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

  • The Union[Callable[[Result], Any], None] is equivalent to Optional[Callable[[Result], Any],]. In fact, I think that newer syntax is recommended Callable[[Result], Any] | None. To fully adopt the newer Python syntax while maintaining backwards compatibility, I'm considering adding the from __future__ import annotations statement to meigas's code. This statement enables the use of forward references in annotations, allowing you to refer to classes and types that haven't been defined yet.

meiga/result.py Outdated
@@ -219,11 +226,11 @@ def handle(

return self

def map(self, transform: Callable) -> None:
def map(self, mapper: Callable) -> None:
Copy link

Choose a reason for hiding this comment

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

Pretty sure one cannot use Callable without type arguments in []. Maybe update it to Callable[..., Any] while we're here?

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 don't know why mypy doesn't warn us about this missing typehint 🤔.

Copy link

Choose a reason for hiding this comment

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

Mypy has pretty tame defaults, to the point that I myself prefer to run it with --strict option (enabling all complaints possible) and then meticulously disabling unwanted options.

Tho forcing this on an unprepared codebase will result in a ton of unexpected red squiggles. Currently counting 33 on meiga/ and 153 on tests/ directories in Meiga repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh thank you for this advice.

meiga/result.py Outdated
@@ -255,3 +262,21 @@ def assert_failure(
)

value = property(get_value)

def set_transformer(self, transformer: Callable[["Result"], Any]):
Copy link

Choose a reason for hiding this comment

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

  • *Result[TS, TF]
  • not sure if quoting the type is required here

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 think this is required as we're typing inside the Result class (is not defined yet)

Screenshot 2023-05-12 at 16 29 34

I don't know how to ad TS and TF within the quotation. I'll try it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to work

def set_transformer(self, transformer: Callable[["Result[TS, TF]"], Any]):



@pytest.mark.unit
def test_should_set_transformer_an_return_transformed_value():
Copy link

Choose a reason for hiding this comment

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

* _and_ instead of _an_? Same with the 2nd test below.

@acostapazo acostapazo merged commit 11a9eef into main May 12, 2023
@acostapazo acostapazo deleted the feature/transform branch May 12, 2023 22:09
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