Considering accessors versus properties on models.

[ghost]

I would like to move most properties on models across to use accessor methods instead of properties. This will probably also involve making most fields private and creating getters for them.

The reason for this is that it allows me to hide whether an operation is using a cached value on the model or whether it is performing a cache lookup behind the method interface. As well as improving consistency elsewhere, it will ensure that introspecting a model instance does not perform a lot of slow operations based on the execution of properties that make cache calls internally.

This would either cover changing just cache calls, or changing all field accessions:

event.channel.id -> event.get_channel().get_id()

This will require at least a partial rewrite of most model-bound tests.

I am still 50-50 as to whether I want to go ahead with this, since it will make stuff more verbose for people switching from other frameworks, and may be considered to be messy, but at the same time the inconsistency between "what is a field and what is a method" which may be messy currently due to Discords ™️ design should be reduced/removed.

Using accessors in Python isn't considered an antipattern per-se either. Many modules follow that approach.

Links to https://discordapp.com/channels/574921006817476608/577602779410071575/749746748615950539

[FasterSpeeding]

Do you have any idea how this would work with builder cache objects? And would we maybe have a method that returns a dict of the raw values or something like that and is on all models?

[ghost]

Do you have any idea how this would work with builder cache objects? And would we maybe have a method that returns a dict of the raw values or something like that and is on all models?

You'd probably have to look at private fields and remove the underscore directly for the constructor to work around this.

[FasterSpeeding]

Do you have any idea how this would work with builder cache objects? And would we maybe have a method that returns a dict of the raw values or something like that and is on all models?

You'd probably have to look at private fields and remove the underscore directly for the constructor to work around this.

If you're fine with looking at private fields then it shouldn't be too hard to convert the data class over then

[ghost]

Do you have any idea how this would work with builder cache objects? And would we maybe have a method that returns a dict of the raw values or something like that and is on all models?

You'd probably have to look at private fields and remove the underscore directly for the constructor to work around this.

If you're fine with looking at private fields then it shouldn't be too hard to convert the data class over then

It is internal to the library. As long as we do not encourage users to do this, we shouldn't have a problem.

Fields are exposed on the assumption we are responsible developers, tests should highlight bad stuff :^)

[Tmpod]

I feel like the cons outweight the pros quite a bit in this case. Python is a syntatically simple language in general, and the ease of use is one of its key features, and certainly one that attracts a lot of people. Making the library super verbose by making everything a call to a get_foo() method will substantially reduce the simplicity and pain-less aspect of this library, which I'm afraid will make people gravitate towards solutions like discord.py more in the future. Even though Hikari is, in my opinion, miles better than discord.py due to its fully typed, documented and tested nature, I'm under the impression that it would level out, or even make Hikari less desireable, as its nice pros would be muffled by the high verbosity and "ugliness" of code using it.
I see how this is a quite complicated decision however. Having these calls would simplify stuff and make everything more consistent... You say it's not an anti-pattern per-se and that many modules use it, but I in all honesty, I fail to recall any. Could you point me some?

Thanks 🅱️iscord

[ghost]

You say it's not an anti-pattern per-se and that many modules use it, but I in all honesty, I fail to recall any. Could you point me some?

Pretty much all standard lib modules like asyncio, thread, logging, do this.

Mainly they use future.result() instead of future.get_result() but the concept is the same.

If people are happy that you have to be mindful of how you use event entities, for example, then I am fine with keeping stuff how it is.

An alternative to this may be using custom overrides of __dir__ to exclude properties on instances.

This fixes the introspection issue by allowing inspect.getmembers to skip stuff we deem slow. We could follow what Python's enum type does, which is only specify specific values (in this case slots only, or attrs fields only).

>>>
>>> class Foo:
...     def bar(self): pass
...     def baz(self): pass
...     def bork(self): pass
...     def __dir__(self): return ["bar", "baz"]
...
>>> import inspect
>>>
>>> inspect.getmembers(Foo())
[('bar', <bound method Foo.bar of <__main__.Foo object at 0x7395e7afa0>>), ('baz', <bound method Foo.baz of <__main__.Foo object at 0x7395e7afa0>>)]
# notice no bork

[ghost]

Might close this for the time being and reopen it if I still feel it is relevant.

[ghost]

@FasterSpeeding @davfsa please.

If we are going ahead with this, we need to look into the pros and cons of doing this and ensure we have it noted down before we make a bad decision.

[davfsa]

As I said through Discord on the day this was opened (should have done it through here) I dont really mind one or another. Its true that properties might be a more "pythonish" approach.

I think its fine to have get_x in a lot of cases (eg, get_channel or get_guild). But for simple or inmutable arguments that wont change (eg id) I think its fine to have them as properties.

[ghost]

I think its fine to have get_x in a lot of cases (eg, get_channel or get_guild). But for simple or inmutable arguments that wont change (eg id) I think its fine to have them as properties.

How would you suggest handling events and inheritance hierarchy in this case?

Consider GuildEvent which has a guild property.

Both GuildAvailable and GuildUnavailable provide this attribute in different ways. The former is always aware of the field directly as it is part of the low-level event payload, whereas the latter has to do a cache call for this (calls the GuildEvent default property).

Should these be properties, getter methods, or just remove this from the base class altogether and provide .guild on an available event and .get_guild() on an unavailable event, in your opinion?

[davfsa]

Should these be properties, getter methods, or just remove this from the base class altogether and provide .guild on an available event and .get_guild() on an unavailable event, in your opinion?

In this case I would use get_guild() for both.

I probably did not explain myself correctly before. I meant if its something that we can assure will always be there (eg id) and we dont have to make any calls for it in any variant of the object, then its a property, else, a get_x. That sound better?

Edit: this might cause a lot of inconsistencies. Tbh, i would first do #49 and try to get them as simple as we can to later implement this

[ghost]

@davfsa suggestions on what that involves?

[ghost]

For now I think we have avoided the issue, so will close.

Will reopen if/when this is needed.