Enhance the TypeHints to return actual Models and not ormar.Model

[collerek]

As described in #111 QuerySet methods should actually return inferred Model subclasses and not the main ormar.Model.

Since I am not an expert in complex python typing any help appreciated.

[aliakhtar]

Yes, great library, but can we please have a release that fixes this issue? Hopefully it also works for objects.get()

[collerek]

There is already PR #132 that should fix all objects.method() calls, which is not all that is to fix, but the most common calls should be handled.

[aliakhtar]

Thanks - is that available on pip? On Tue, 23 Mar 2021 at 8:48 AM, collerek @.***> wrote:

There is already PR #132 https://github.com/collerek/ormar/pull/132 that should fix all objects.method() calls, which is not all that is to fix, but the most common calls should be handled.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/collerek/ormar/issues/112#issuecomment-804595034, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABIUCOBQYLQ2CF2LBY3ONLTTFAFQBANCNFSM4YAG2KDQ .

[collerek]

If you mean the PR, not yet, need to finish the docs and will merge and publish this probably today.

As of now you can install from source (gh branch) I guess.

[aliakhtar]

Awesome - Love it!

I think Model.objects.get() should return an optional instead of raising a NoMatch - do you agree @collerek ? If so I can do a PR for that.

[collerek]

@aliakhtar As of now I prefer the exception, since None is a perfectly valid return type, especially for aggregated functions like max or min. Without exception you don't know if it's an actual value from the database or there is no match for your query criteria.

Following pep8: Explicit is better than implicit, and for me returning None where there is no record in db with regard to ORM is kind of an implicit error silencing, and it makes testing and debugging more difficult (did I make a typo and there is no record or something was saved in a wrong way and the data is corrupted etc.)

If you don't like it you can very easily write a decorator/wrapper/helper that will silence this error and return None, but other way around (getting to know if it's db value or error) would be pretty hard without subclassing and overwriting implementation (which of course you can also do).

[aliakhtar]

We could keep all the other exceptions and only return None if no record is found? IMO returning None if no record is found is pretty explicit, and in code it makes it a lot easier to check if no record was found vs doing a try / except.

[collerek]

@aliakhtar that's not so simple as they rely on the same exception :)

Moreover this would be a breaking change that would have to wait for next bigger release, to add even more I like the exception approach more and there is quite some amount of users that do too.

But since it's not the first time someone asks me for this functionality I see a place for this but more like a setting then a permament change. I can add a setting on Model's Meta class (that can be easily shared by all models through BaseMeta approach if you use one database only (check best practices in docs)), something like exception_on_no_match=True that you can change to False.

Can you please open an enhancement issue for this?

[aliakhtar]

What about a new method called get_if_exists with the type signature to return optional instead? Happy to send a PR for it.

P.S I have a deadline to meet but the bug I mentioned in my other issue is stopping me.. none of the select queries are working

On Tue, Mar 23, 2021 at 7:50 PM collerek @.***> wrote:

@aliakhtar https://github.com/aliakhtar that's not so simple as they rely on the same exception :)

Moreover this would be a breaking change that would have to wait for next bigger release, to add even more I like the exception approach more and there is quite some amount of users that do too.

But since it's not the first time someone asks me for this functionality I see a place for this but more like a setting then a permament change. I can add a setting on Model's Meta class (that can be easily shared by all models through BaseMeta approach if you use one database only (check best practices in docs)), something like exception_on_no_match=True that you can change to False.

Can you please open an enhancement issue for this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/collerek/ormar/issues/112#issuecomment-804965831, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABIUCOH42RLNL2CDPRPW6TTTFCTB3ANCNFSM4YAG2KDQ .

[collerek]

@aliakhtar - in 0.10.1 I added get_or_none()

[aliakhtar]

That's awesome, and very fast. Thanks!

^{Almost wish you hadn't done it so I could send a PR 😛}