core: nmea-injector: Make settings persistent between sessions #986
Conversation
rafaellehmkuhl
force-pushed
the
add-settings-to-nmea-injector
branch
from
8dbb729 to
168cbba
Compare
rafaellehmkuhl
force-pushed
the
add-settings-to-nmea-injector
branch
4 times, most recently
from
3b3af1d to
1fea99b
Compare
|
|
||
| def __init__(self) -> None: | ||
| self._socks: Dict[NMEASocket, Union[asyncio.AbstractServer, asyncio.BaseTransport]] = {} | ||
| self._settings_manager = Manager("NmeaInjector", SettingsV1) |
There was a problem hiding this comment.
we are using lowercase for our settings name.
| else: | ||
| raise UnsupportedSocketKind(f"Got {sock.kind}. Expected one of: {[kind.value for kind in SocketKind]}.") | ||
| self._socks[sock] = server_socket | ||
| settings_spec = NmeaInjectorSettingsSpecV1.from_spec(sock) |
There was a problem hiding this comment.
It would be better to have such method encapsulate over a free function or inside the NMEASocket class, the reason behind this is that each settings version will need to implement such function unnecessary, like V1, V2, v3, so every time that we update the settings such method will need to exist.
If we move it to a free function or inside the NMEASocket class, the function will only need to exist in a single place and be updated when the settings version change, over duplicating such method over and over for each settings.
There was a problem hiding this comment.
Agree. Was following the same pattern as the Bridget implementation, but this seems better.
| component_id = pykson.IntegerField() | ||
|
|
||
| @staticmethod | ||
| def from_spec(spec: "NmeaInjectorSpec") -> "NmeaInjectorSettingsSpecV1": # type: ignore |
There was a problem hiding this comment.
This should be NMEASocket when reading async def add_sock(self, sock: NMEASocket) -> None:
rafaellehmkuhl
force-pushed
the
add-settings-to-nmea-injector
branch
2 times, most recently
from
8812683 to
a125f81
Compare
|
@patrickelectric I'm waiting for the image build to test with the modifications. |
This will make people lose it currently settings, but since we already had the "non-persistent" banner on the frontend till now, that's what they were expecting.
rafaellehmkuhl
force-pushed
the
add-settings-to-nmea-injector
branch
from
a125f81 to
e259069
Compare
I tested and eveything appears to be working. |
2 participants
Fix #708
Image available.