django-stubs icon indicating copy to clipboard operation
django-stubs copied to clipboard

Avoid returning None from get_field_related_model_cls

Open SingingTree opened this issue 1 year ago • 4 comments

https://github.com/typeddjango/django-stubs/pull/1495 updated get_field_related_model_cls to raise UnregisteredModelError rather than returning None for failure paths. However, None can still be returned if the initial retrieval of related_model_cls returns None.

This patch adds a check to see if the initial retrieval has got a None and then raises the appropriate error rather than letting None be returned.

Related issues

Closes https://github.com/typeddjango/django-stubs/issues/1953

SingingTree avatar Feb 18 '24 22:02 SingingTree

Do you think that adding a test case is a good idea here?

yeah I think it would be great to add a test case to tests/typecheck/fields/test_related.yml with a minimal reproducible example :pray:

UnknownPlatypus avatar Feb 19 '24 11:02 UnknownPlatypus

Updated to shift location of check.

I'm open to suggestions re tests. The scenario I'm encountering this in is some proprietary code that I'm yet to reduce to something that nicely fits into a test here.

SingingTree avatar Feb 19 '24 22:02 SingingTree

@SingingTree

random1st avatar Feb 22 '24 07:02 random1st

I've been traveling, and will make some time to get my branch updated over the next couple of days.

SingingTree avatar Feb 22 '24 09:02 SingingTree

I've reverted the changes back to the first iteration of PR. I'm afraid I haven't had any luck in reducing the faulting code to a test case.

FWIW, the scenario I'm seeing this hit in is using django taggit. I'm not certain what's going on under the hood that's resulting in the unhappiness however.

SingingTree avatar Feb 25 '24 07:02 SingingTree