-
Notifications
You must be signed in to change notification settings - Fork 156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added monitor state TypedDict definitions #1206
base: develop
Are you sure you want to change the base?
Added monitor state TypedDict definitions #1206
Conversation
I definitely like the general idea of this. I think I'll need to delve into the code for some of those Optional[None] to get a feel for how much work it would be to do the extra None checking. |
monitor_state_dict.py
Outdated
Manufactured: DefaultDict[str, int] | ||
|
||
# Ship | ||
ShipID: Optional[str] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gets compared to the CAPI data ship->id so we'd need to be sure the CAPI never brainfarts and returns that.
Might be a wrinkle with EDO starting on foot in a new account ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im using -1 as a default value, should be a decent enough sentinel I think. its ints on both sides otherwise (and if it wasnt we'd have issues anyway) -- comparisons looked fine where I checked them (searched the codebase for ShipID
The one thing about all my comments re: changing from All it takes is one plugin dev doing I'm inclined to just do that. |
ab2f5fb
to
3320cee
Compare
Something that I want to add, a heavy mode that checks types on monitor.state after each journal line, to ensure that things are okay |
if (suit_loadouts := data.get('loadouts')) is None: | ||
logger.warning('CAPI data had "suit" but no (suit) "loadouts"') | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is the change of behaviour you mentioned on Discord.
Before this if CAPI says no suit loadouts we'll None our copy. With this, it won't.
So now the question is do we really distrust CAPI that much ? I'm inclined to allow this change as the Journal is in much better shape and we should always be picking things up properly from it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah Im undecided here. The other option is to blank our loadouts and return?
BackpackJSON: MutableMapping[str, Any] # Direct from Game | ||
ShipLockerJSON: MutableMapping[str, Any] # Direct from Game | ||
|
||
SuitCurrent: Optional[SuitDict] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without checking the code that uses this, I am wondering if we can't default this to {}
and lose the Optional
. Same for SuitLoadoutCurrent below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
specifically because the "default" cannot be {}
, it must be a dict that matches SuitDict
, so the minimum default is:
{
'name': '',
'locName': '',
'weaponrackId': 0,
'id': None,
'suitId': 0,
'mods': [],
}
which is not going to be falsy
credits defaulting to 0 is fine
b2a1bb1
to
2e245f6
Compare
This isnt going to happen, the introspection required isnt there, half of the types will be strings |
This probably is going to wholly need a rewrite at this point. |
@A-UNDERSCORE-D you mentioned above this will need a rewrite, should I close this PR at this point then? |
Likely yeah. It was intended to bring safety to the main loop, but we were changing a lot at the time so we left it. It may be salvageable in some places |
It was kept around as easy reference for maybe completing the work at some point. The issue it was designed to address is that the monitor state dictionary is just that, a dictionary, and as such it's all too easy to mis-type/capitalise a member of it and thus introduce a bug. We can't just wholesale change it to an object without huge impact on plugin developers, so this was an attempt at a way to move towards that whilst still supporting dictionary key access. |
These don't actually change types or make anything more safe. What they
WILL do is make mypy and co complain loudly if we make mistakes.
Im looking for comments on this, as there are a few issues currently
For one, as it stands we None a lot of things that "will" exist always at runtime, which while we know are safe, the type checker cannot, thus a large number of errors appear.
There are two ways to fix this:
Im not sure which to go with, but either way this will prevent typo issues in future and do some type checking.