Add type annotations to constructor parameters in Literal by veyndan · Pull Request #1498 · RDFLib/rdflib (original) (raw)

I think the best way to resolve this is to start from where we agree:

I definitely agree that we should raise a ValueError when the value is of the right type but the value is wrong — no qualms there.

If x = Literal("foo",lang="999") should raise a ValueError, then what should happen when someone runs x = Literal("foo",lang={}) ? Should it construct a Literal (i.e. should constructor complete with no exception and return an object which is essentially corrupted), and if so why? And if it should not, then what is the most reasonable expectation when running x = Literal("foo",lang={})?

To me, I do expect x = Literal("foo",lang={}) to raise an exception if Literal's constructor is going to do value space validation for lang, maybe expecting it to raise a TypeError is overly specific, maybe it would be okay if it raised either a TypeError or a ValueError, but it is unclear what other type of exception would be more appropriate, definitely RuntimeError would be wrong, Exception would be overly broad, and similarly all other exceptions are significantly less appropriate than TypeError or ValueError.

I think there is maybe some argument for providing a way to disable all value space validation for cases where someone wants best effort processing of broken RDF, but given value space validation is done, and it is always done, I think it is very reasonable to always expect an error when the provided value falls outside of the value space, and {} does fall outside of the value space, and thus I do expect it to generate an error, and the most reasonable errors for it to generate are TypeError or ValueError, with TypeError being more reasonable than ValueError - but definitely, given value space validation is expected, it would be wrong for x = Literal("foo",lang={}) to succeed without raising any exception.

Consider this code:

from typing import Any, Optional, Type

import pytest

class EvenNumber: value: int

def __init__(self, value: int) -> None:
    if value % 2 != 0:
        raise ValueError(f"{value} is not divisible by 2")

@pytest.mark.parametrize( "value, error", [ (2, None), (1, ValueError), ({}, TypeError), ([], TypeError), ("not a number", TypeError), ], ) def test_value_space(value: Any, error: Optional[Type[Exception]]) -> None: if error is not None: with pytest.raises(error): EvenNumber(value) else: EvenNumber(value)

This generates the following output:

$ poetry run pytest tests/test_valspace.py ============================================================================ test session starts ============================================================================ platform linux -- Python 3.9.9, pytest-6.2.5, py-1.10.0, pluggy-0.13.1 rootdir: /home/iwana/sw/d/gitlab.com/aucampia/pvt/scratchpad/tech/py3, configfile: pyproject.toml plugins: subtests-0.5.0, cov-2.12.1 collected 5 items

tests/test_valspace.py::test_value_space[2-None] PASSED [ 20%] tests/test_valspace.py::test_value_space[1-ValueError] PASSED [ 40%] tests/test_valspace.py::test_value_space[value2-TypeError] PASSED [ 60%] tests/test_valspace.py::test_value_space[value3-TypeError] PASSED [ 80%] tests/test_valspace.py::test_value_space[not a number-TypeError] PASSED [100%]

============================================================================= 5 passed in 0.02s =============================================================================

I did no explicit type checks here, but I still get TypeError here, and what is more, if I expect that a class constructor is going to do runtime value space validation, I do at the very least expect an exception when I give it a value outside of that value space, regardless of the type, to me what would be completely unexpected and wrong is if a constructor fails for something which is of the right type but outside of the value space, but succeeds for the wrong type.

And actually if you did this, mypy would error out, because warn_unreachable is enabled for rdflib, and the above would be unreachable as far as mypy is concerned, and within this is probably the best answer I can give you to this question, if mypy determines that the type checking is unreachable, then it probably should not be there.

I don't believe that's true based on the documentation where it says the warn_unreachable warning will be silenced when "the unreachable statement is a raise statement".

Today I learn, I guess this behaviour makes some sense, regardless though, I still do not think that the following is needed:

if not isinstance(lang, str): raise TypeError(f"Type of lang {lang!r} is invalid")

And again I mainly don't think it is needed because a type error will already be raised if lang is not a string with your code as is.

With the raising of TypeError when the type is wrong, I think if there's some way that mypy could disallow unnecessary checks (from the sounds of it, what you were proposing that warn_unreachable should do) I think that'd be awesome

I think maybe pyright can do it, but I am not sure that really is not that big a problem, anyway not at the moment. And I think it is also good to consider that a lot of rdflib users don't use mypy, and this may remain the case for a long time, because rdflib is used a lot in academic and scientific settings.