Change try_components methods in order to not return an iterable.

[Lehnart]

Hey,

Here is a proposition for the try_component(s) methods. I don't understand why they are supposed to return an iterable since there are specifically designed to return a single component (or a list of single components for the try_components method).

I find it clearer to return either the Component(s) either None if they are not existing. You have an extra "if" compared to the iterator method but I think it is more readable.

Let me know what you think of this!

[benmoran56]

Thank you for opening the PR! This pattern already exists, which I think is fairly similar to your proposal:

if self.world.has_component(ent, Stun):
    stun = self.world.component_for_entity(ent, Stun)
    stun.duration -= dt

The only reason for the existence of the try_component method was to save on lines of code, even though it might look a little strange:

for stun in self.world.try_component(ent, Stun):
    stun.duration -= dt

Think about your proposed change, the current simplified usage could be maintened, if you use the walrus operator. Of course, that would preclude Python 3.7, which is still in use on some platforms.

for stun := in self.world.try_component(ent, Stun):
    stun.duration -= dt

What do you think?

[benmoran56]

I just realized there is a mistake in the README as well, but I'll fix that :sweat_smile:

[Lehnart]

Thank you for opening the PR! This pattern already exists, which I think is fairly similar to your proposal:

if self.world.has_component(ent, Stun):
    stun = self.world.component_for_entity(ent, Stun)
    stun.duration -= dt

I agree. My proposal was about saving a call to self._entities[entity], which I thought was the purpose of try_component. In your example, you have to call twice the entity dictionary (one in has_component and one in component_for_entity). Anyway, performance is not an issue with esper so I can completely use your suggestion.

The only reason for the existence of the try_component method was to save on lines of code, even though it might look a little strange:

for stun in self.world.try_component(ent, Stun):
    stun.duration -= dt

Think about your proposed change, the current simplified usage could be maintained, if you use the walrus operator. Of course, that would preclude Python 3.7, which is still in use on some platforms.

for stun := in self.world.try_component(ent, Stun):
    stun.duration -= dt

Oh great, I didn't know about this operator. But, you meant this way right?

if stun := self.world.try_component(ent, Stun):
     stun.duration -= dt

I don't get the point to keep an iterable as return type.

What do you think?

Yeah, I think it's the best solution. I'll modify the merge request in this way if you agree.

[Lehnart]

I just realized there is a mistake in the README as well, but I'll fix that 😅

Oh you mean I've added a mistake? I meant to update the readme accordingly to my pull request.

[benmoran56]

Oh you mean I've added a mistake? I meant to update the readme accordingly to my pull request. No, sorry for the confusion. I just spotted a mistake that was already there.

[Lehnart]

Actually, I have nothing to change in the merge request :). I had in mind to include the walrus operator thanks to a from future import walrus but it's useless on the library side :D.

In the end, I'm not sure if your proposition was to not merge this and use the walrus operator on my side, or to merge this and so everyone will use the walrus operator! I'm fine with both. I let you merge/close.

[benmoran56]

Sorry for the delay.

What I meant to say was that your change makes sense if combined with the walrus operator. Without the walrus operator, it adds an extra line of typing (which is against the idea of the try_ methods in the first place).

I'm leaning towards merging it in, even though Python 3.7 is still in use on some platforms.