Skip to content
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

Draft
wants to merge 27 commits into
base: develop
Choose a base branch
from

Conversation

A-UNDERSCORE-D
Copy link
Contributor

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:

  • do None checks everywhere (which is probably what we SHOULD do but given the shape of monitors interactions with this, will be messy and create a lot more confusing code)
  • lie to the type checkers -- By telling them the real type of $thing, setting None anyway and telling them to ignore that we set None

Im not sure which to go with, but either way this will prevent typo issues in future and do some type checking.

@Athanasius
Copy link
Contributor

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 Show resolved Hide resolved
monitor_state_dict.py Outdated Show resolved Hide resolved
monitor_state_dict.py Outdated Show resolved Hide resolved
monitor_state_dict.py Outdated Show resolved Hide resolved
monitor_state_dict.py Outdated Show resolved Hide resolved
monitor_state_dict.py Outdated Show resolved Hide resolved
Manufactured: DefaultDict[str, int]

# Ship
ShipID: Optional[str]
Copy link
Contributor

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 ?

Copy link
Contributor Author

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

monitor_state_dict.py Outdated Show resolved Hide resolved
monitor_state_dict.py Outdated Show resolved Hide resolved
monitor_state_dict.py Outdated Show resolved Hide resolved
monitor_state_dict.py Outdated Show resolved Hide resolved
@Athanasius
Copy link
Contributor

The one thing about all my comments re: changing from None to '', 0, or other type-appropriate falsey value is that whilst we can be sure core code will work with it, we don't know what plugins are doing with the state values.

All it takes is one plugin dev doing if state['ShipName'] is not None or similar and we're obligated to bump our major version number if we make those changes.

I'm inclined to just do that.

monitor_state_dict.py Outdated Show resolved Hide resolved
monitor_state_dict.py Outdated Show resolved Hide resolved
monitor_state_dict.py Outdated Show resolved Hide resolved
monitor_state_dict.py Outdated Show resolved Hide resolved
monitor_state_dict.py Outdated Show resolved Hide resolved
monitor_state_dict.py Outdated Show resolved Hide resolved
monitor_state_dict.py Outdated Show resolved Hide resolved
monitor_state_dict.py Outdated Show resolved Hide resolved
@A-UNDERSCORE-D
Copy link
Contributor Author

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
Copy link
Contributor

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.

Copy link
Contributor Author

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]
Copy link
Contributor

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.

Copy link
Contributor Author

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

monitor.py Outdated Show resolved Hide resolved
monitor.py Outdated Show resolved Hide resolved
monitor.py Outdated Show resolved Hide resolved
monitor.py Show resolved Hide resolved
@A-UNDERSCORE-D
Copy link
Contributor Author

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

This isnt going to happen, the introspection required isnt there, half of the types will be strings

@A-UNDERSCORE-D A-UNDERSCORE-D marked this pull request as ready for review August 7, 2021 18:17
@A-UNDERSCORE-D
Copy link
Contributor Author

This probably is going to wholly need a rewrite at this point.

@Rixxan
Copy link
Contributor

Rixxan commented Jul 27, 2023

@A-UNDERSCORE-D you mentioned above this will need a rewrite, should I close this PR at this point then?

@A-UNDERSCORE-D
Copy link
Contributor Author

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

@Athanasius
Copy link
Contributor

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.

@Rixxan Rixxan marked this pull request as draft April 27, 2024 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants