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

EDDN: schema - ApproachSettlement #1450

Closed
Athanasius opened this issue Feb 8, 2022 · 3 comments
Closed

EDDN: schema - ApproachSettlement #1450

Athanasius opened this issue Feb 8, 2022 · 3 comments

Comments

@Athanasius
Copy link
Contributor

Ref: https://github.com/EDCD/EDDN/blob/beta/schemas/approachsettlement-README.md

Currently available on the EDDN Beta service.

@Athanasius
Copy link
Contributor Author

NB: This was worked on in #1452 BUT I've realised some of that was misguided, or at the very least monitor.py needs reviewing to ensure that both monitor.system and monitor.state['StarSystem'] definitely stay synchronized.

I think it's possible that they'll be incidentally synchronized versus the actual use, i.e. after monitor.system is reset to None the other won't be used until monitor.system once more has a value. But let's be sure.

Also, when working on #1452 it took me far too long to remember/realise that Plugin journal_entry() gets passed monitor.system as its system parameter. So it's valid to think that adding monitor.state['StarSystem'] isn't even necessary. HOWEVER, I think I'd be in favour of just moving all use of monitor.system into monitor.state['StarSystem'] instead. To be honest, all of monitor.py could do with reviewing and having most, if not all, such tracking in the monitor.state dictionary.

Also, with respect to #1228 , other than the backwards compatibility layer, I absolutel think we should not explicitly pass system(_name) into the plugin call point when we have (monitor.)state being passed in, and that can contain (near) everything necessary. Heck, I'm not even sure it needs cmdr(_name) or is_beta passing in!

@Athanasius
Copy link
Contributor Author

To have this addressed ASAP for 5.3.0 I'll be submitting a PR that simply reverts the monitor.py addition of state['StarSystem']. It's already not actually used by anything (core plugins or other code).

Shuffling things into monitor.state (for availbility) is more of a #1206 thing, so also not for 5.3.0.

And obviously #1228 is its own thing.

So, after next little merge this can be closed for 5.3.0 release.

@Athanasius
Copy link
Contributor Author

Released in 5.3.0-beta11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant