sc2reader icon indicating copy to clipboard operation
sc2reader copied to clipboard

TargetUnitCommandEvent.target_unit vs target?

Open zimri-leisher opened this issue 3 years ago • 1 comments

Discussed in https://github.com/ggtracker/sc2reader/discussions/147

Originally posted by zimri-leisher July 28, 2021 I'm relatively new to this library but I think I may have found a small issue: in the TargetUnitCommandEvent class definition, there is a field called target_unit that apparently stores a reference to the unit being targeted. However, in the ContextLoader class, in the handleTargetUnitCommandEvent method, it puts a reference to the target unit into the target field. This caused a small issue for me before I figured it out, and I know it's not a big deal, but I figured I'd better post here.

zimri-leisher avatar Jul 28 '21 18:07 zimri-leisher

Interesting. Specifically, you're referring to the definition here

https://github.com/ggtracker/sc2reader/blob/c2a5407cb13fc41e2b44af07ae2a7ba48ffb7695/sc2reader/events/game.py#L330

and corresponding effort to populate it here

https://github.com/ggtracker/sc2reader/blob/c2a5407cb13fc41e2b44af07ae2a7ba48ffb7695/sc2reader/engine/plugins/context.py#L72

and here

https://github.com/ggtracker/sc2reader/blob/c2a5407cb13fc41e2b44af07ae2a7ba48ffb7695/sc2reader/engine/plugins/context.py#L86

Right?

Given that there are references to target in other places like

https://github.com/ggtracker/sc2reader/blob/c2a5407cb13fc41e2b44af07ae2a7ba48ffb7695/sc2reader/events/game.py#L251

and I presume that most people are using target, I think probably the right fix is to change the class definition to declare target not target_unit. What do others think?

@zimri-leisher Can you open a pull request updating that?

StoicLoofah avatar Aug 10 '21 04:08 StoicLoofah