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

Resolve Pylance type checking warning. #182

Closed
harttraveller opened this issue Mar 28, 2024 · 2 comments
Closed

Resolve Pylance type checking warning. #182

harttraveller opened this issue Mar 28, 2024 · 2 comments

Comments

@harttraveller
Copy link

harttraveller commented Mar 28, 2024

Thanks for this great python module! I'd like to submit a requested change to resolve an issue with Pylance. This bug will become more difficult to deal with once the .value method is removed, per this issue: #123

Description

When using the .ok_value and .err_value - Pylance detects an error where there is none. This does not raise any Exceptions, nor does it lead to unexpected code behavior, but it means that if you don't use the currently deprecated .value property then the type checker will identify an error where no error exists.

Example

Here is some code which runs as expected.

from result import Result, Ok, Err


def validate_number_is_even(x: int) -> Result[str, str]:
    """
    NOTE: This is a sample function purely for the purpose of demonstrating result/pylance incongruence.

    Returns `Ok` if number is even, `Err` if number is odd.
    """
    return Err("Failed: Odd Number") if x % 2 else Ok("Passed: Even Number")


expected_ok = validate_number_is_even(x=2)

if expected_ok.is_ok():
    print(expected_ok.ok_value)

expected_err = validate_number_is_even(x=3)

if expected_err.is_err():
    print(expected_err.err_value)

Here is what the code looks like in VSCode, with Pylance and Error Lens enabled:

SCR-20240328-oyen

Proposed Fix

This can be resolved by adding the respective properties to the Ok and Err class, but having them simply raise Exceptions. Pylance no longer detects an error if we add the following properties. I've omitted most of the code from the Ok and Err class to avoid clutter.

New Ok Property

class Ok(Generic[T]):
    @property
    def err_value(self) -> NoReturn:
        """
        Raise an error since this type is `Ok`
        """
        raise AttributeError("Cannot access 'err_value' on 'Ok'")

New Err Property

class Err(Generic[E]):
    @property
    def ok_value(self) -> NoReturn:
        """
        Raise an error since this type is `Err`
        """
        raise AttributeError("Cannot access 'ok_value' on 'Err'")

Temporary Fix

Currently, I'm using the .value property and simply adding this code to all the __init__.py files in the modules I'm developing:

import warnings

warnings.filterwarnings(
    "ignore",
    category=DeprecationWarning,
    message="Accessing `.value` on Result type is deprecated, please use `.ok_value` or `.err_value` instead",
)

This ensures Pylance/VSCode do not detect non-existent problems, without raising the deprecation warning. I thought I would mention this now, as in reviewing the other GitHub issues, I found the note about how the .value method will soon be removed.

@francium
Copy link
Member

francium commented Mar 28, 2024

This is just a quick reply, but I'll read everything else you wrote later. Most likely you're just running into a limitation of the language server.

If done like this, then you get correct type checking,

expected_ok = validate_number_is_even(x=2)

if isinstance(expected_ok, Ok):
    print(expected_ok.ok_value)

expected_err = validate_number_is_even(x=3)

if isinstance(expected_err, Err):
    print(expected_err.err_value)

Someone ran into this earlier, #145 (comment)

This is documented in the README, but it's in the middle, so it may be easy to miss. Search for "The benefit of isinstance is better type checking that type guards currently do not offer," in the README to find that section.

@francium
Copy link
Member

francium commented Mar 29, 2024

This can be resolved by adding the respective properties to the Ok and Err class, but having them simply raise Exceptions

This is not the direction we're going towards with this library. The design of having ok_value and err_value only exist on their respective result types was intentional to make the type checker report errors if you try to access the property on the wrong type.

See #37, #121

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

No branches or pull requests

2 participants