cleanup + bugfixes

[don-de-marco]

I'm doing some long-overdue code cleanup to have this plugin finally adhere to pep8 (or mostly, at least). Also fixing some bugs I find underway.

I will probably be touching every other line in each file so you probably want to merge this branch regularly to reduce the chance of conflicts

[don-de-marco]

@ABaumher I urge you to please give feedback on this before you do any more changes. The amount of merge conflicts is already getting out of hand

[ABaumher]

I don't get notifications for prs, so i missed it. Should figure out how to change that. I'll look asap

[ABaumher]

A couple thoughts on pep standard:

I'm aware python prefers 80 character limits per line, i'm of the persuasion that breaking up a line at an arbitrary limit is less readable than finishing a particular block or comment on one line (within reason ofc).

Default values for function definitions should use the same spacing as every other operator (except type hints, which are special. In regex form, these should be: \s=\s).

If imports or a list iinitializer is relatively small, it can be a one-liner. Personal preference, if i get overruled, so be it.

I use extra line breaks in functions to group related logic - perhaps more frequently than the standard world like but imo that's easier to parse.

But everything else should probably get fixed, lol. I tend to add imports ad-hoc (and will probably continue to do so because it's faster), but alphabetizing them will be necessary eventually. I'm used to c-like syntax so forgive me for all the wrapped if statements XD. If you want to snake_case the rpc calls, go for it.

[ABaumher]

44 My progress on tracking down the messages we send and receive so far. A lot of this is educated guesses, but i'll get to confirming it at some point. will likely merge the changes i make with some sort of "debug" flag similar to LOG_SENSITIVE_DATA, but i'm not doing this for a few days at least. it'll be on a different branch and PR'd in at some point. probably could flag you for review so you can point out my inevitable PEP failures.

[don-de-marco]

I'm aware python prefers 80 character limits per line, i'm of the persuasion that breaking up a line at an arbitrary limit is less readable than finishing a particular block or comment on one line (within reason ofc).

I disabled the line limit check (E501) because the 80 character limit doesn't do us any good.

Default values for function definitions should use the same spacing as every other operator (except type hints, which are special. In regex form, these should be: \s=\s).

I disagree with this because you want to keep the function definitions as concise as possible. In my opinion, introducing spaces around default value assignments (without type indicators) breaks the visual bond between a parameter and it's default value.

If imports or a list iinitializer is relatively small, it can be a one-liner. Personal preference, if i get overruled, so be it.

That's situational for me, too. For the imports I usually just stick what vscode produces so that I don't have to "fix" it everytime I (accidentally) let vscode reorganize the imports.

I use extra line breaks in functions to group related logic - perhaps more frequently than the standard world like but imo that's easier to parse.

My take on this is opposite and I'd like to keep the visual bonds between code fragments intact. But I see where you're coming from. Just tweak the flake config so that it won't complain anymore if you want to allow both.

Also, the order the messages are packed in a multi is not guarenteed, which means some logic that we expect to occur before others (i.e. the ClientLoginResult confirming steam id before using that id to check who the owner is for the LicenseList) is flawed and needs to wait for the multis to finish. There is a means to do callbacks on these, which should help fix this as well. I think the old FoG code had a few await calls to enforce this that have since been lost.

Can't we do something like this:

await self._msg_handler.send(..., expected_answer=CMsgResponseToRequest)

and then do the waiting inside msg_handler? Since you implemented the message queue, having it this way should be relatively easy to implement. I honestly and wholeheartedly oppose doing this awful send-message-await-event intertwined style of code we had before. The old variant severely disrupted the logic of the code since you never knew where exactly the waiting happens

[ABaumher]

I don't disagree the old event based calls sucked, but the problem i don't know how else to address is multis (and even worse, nested multis). The event trick i use is a dictionary of source job id to a future (plus Metadata so we get the expected response if it's wrapped in a multi. SteamKit does the same trick, but doesn't require the Metadata because they have genetic static typing). Unfortunately afaik all items in a multi have the same source job id.

To be clear, I'm not suggesting the callback handlers, instead, the "proper" use of events. Wait until an event occurs, then call an event completed. Anybody can subscribe to that event completed state and then do further processing since they know the data is now valid.

It'd be more like await self._local_storage_cache.all_licenses_processed_event.wait() Multiple sources can wait for this, the intention is clear (wait for event to complete) and setting it to complete when done is trivial

Afaik you can't send info along an event set in python (could be wrong) so i wrapped the event in a callback and passed the multi header back. Likely a better way

[ABaumher]

>12k licenses has an issue where it becomes a multi. Pics product info likes sending multis. Here's how i propose fixing issues like this:

1. Process the individual message.
2. Create a task that awaits a "multi done processing" event for that group of messages.
3. Ensure it's only created once.
4. Once the last multi packet is processed, set that event.
5. The task wakes up, knows all of those identical messages (with different data) have been handled, and does whatever it needs to in order to finish processing all the data.
6. Set a processing complete, let the normal code waiting on it resume execution

I'd prefer higher level events than what we have, (like C# or Java where an event object can be overridden to add local members), but a cursory search did not turn up anything promising. I can fake it with callbacks but those get ugly really quickly, as you mentioned

[ABaumher]

Difference between asyncio.event and asyncio.future is an event can have multiple subscribers. Useful if several different responses appear multiple times in the same multi (they all aren't guaranteed to be complete until the multi is fully processed, and it's more understandable if we don't have one global task that may handle multiple different responses). Unfortunately, an event essentially only returns a bool, a future can return anything.

In theory i could just create a high level event that returns anything like a future (with added generic type hints) but under the hood just wraps an event . It's actually really simple, but idk if it's overkill.

[ABaumher]

To not cause merge conflicts, here's a generic event.

from asyncio import Event
from typing import Generic, Optional, TypeVar

T = TypeVar("T")
class GenericEvent(Generic[T]):  # noqa: E302

    def __init__(self):
        self._event: Event = Event()
        self._value: Optional[T] = None

    async def wait(self) -> T:
        await self._event.wait()
        return self._value

    def set(self, value: T) -> None:
        self._value = value
        self._event.set()

    def clear(self) -> None:
        self._event.clear()
        self._value = None

    def is_set(self) -> bool:
        return self._event.is_set()

this was unique/conceptually challenging enough to ignore burnout but i'm not touching anything else, lol. LMK what you think.

Edit: PEP-8 Compliance :). Well, unless they want TypeVar two lines above the class, imo this is an exception to that rule. So i added a flake ignore on it, lol

[ABaumher]

If you want i can merge this now and you can make more PRs as you go, or i can leave this open and you can re-request a review whenever you make changes. Like i said, i intend to be relatively hands off for a little while to recharge.

[don-de-marco]

I'm gonna roll this up from the back

If you want i can merge this now and you can make more PRs as you go, or i can leave this open and you can re-request a review whenever you make changes. Like i said, i intend to be relatively hands off for a little while to recharge.

Yes, please merge. The longer we wait, the more merge conflicts I need to resolve.

I'd prefer higher level events than what we have, (like C# or Java where an event object can be overridden to add local members), but a cursory search did not turn up anything promising. I can fake it with callbacks but those get ugly really quickly, as you mentioned

To not cause merge conflicts, here's a generic event.

from asyncio import Event
from typing import Generic, Optional, TypeVar

T = TypeVar("T")
class GenericEvent(Generic[T]):  # noqa: E302

    def __init__(self):
        self._event: Event = Event()

<snip>

    def is_set(self) -> bool:
        return self._event.is_set()

this was unique/conceptually challenging enough to ignore burnout but i'm not touching anything else, lol. LMK what you think.

Edit: PEP-8 Compliance :). Well, unless they want TypeVar two lines above the class, imo this is an exception to that rule. So i added a flake ignore on it, lol

You can always subclass any other class in Python. And as long as you're not overwriting elements of the parent class, you'll not run into problems in situations where you'd need to cast to the parent type in other languages. So you can just do this and should be fine (ignoring most of the type hints):

class GenericEvent(Event):
    def __init__(self):
        super().__init__()
        self._value: Optional[T] = None

    async def wait(self) -> T:
        await super().wait()
        return self._value

    def set(self, value: T) -> None:
        self._value = value
        super().set()

    def clear(self) -> None:
        super().clear()
        self._value = None

Difference between asyncio.event and asyncio.future is an event can have multiple subscribers.

Which makes sense because events are for flow control/synchronization while Futures are only meant to be a "this will eventually hold the requested value" object ;)

[ABaumher]

IIRC python allows multiple inheritance, but it's weird. Can we do GenericEvent(Event, Generic[T])

[don-de-marco]

Yeah, you can do that. But I must read up on how you call the parent constructors correctly in that case. I believe it's just super().__init__() as usual but I remember some pitfalls

[ABaumher]

I believe it's just super().__init__() as usual but I remember some pitfalls

Reading up on MRO makes my brain hurt lol. I think since they don't have any potential diamond problems super on the base event calls will work. Not sure about __init__ though

[ABaumher]

fun fact, underlying CPython implementation of asyncio.Event uses _value. I'm not sure if the name would clash, so i used _generic_value instead. I'm going to do a commit here, you can just cherry-pick it.