def-gthill / lexurgy

A high-powered sound change applier
GNU General Public License v3.0
42 stars 5 forks source link

Support reloading sound changes in server mode #76

Closed Sirraide closed 5 months ago

Sirraide commented 5 months ago

This adds two additional request types to the CLI’s server mode to enable (re)loading a new set of sound changes from a file or directly from the request. Essentially, you can now do this:

{"type": "load_string", "data": {"path": "sound changes go here" }}

I’ve also made the CHANGES parameter optional now that you can load the changes after starting the server. Furthermore, the previous request format was not exactly amenable to extension, so I’ve refactored it to use a type+data schema. The old request format with all the data at the top level is still supported for backwards compatibility, albeit not for any of the new requests.

I personally need this feature for a project I’m working on, so I thought I might as well upstream it in case you’re interested in it as well. I don’t typically work with Kotlin though, so I’m not sure whether my implementation of this is idiomatic or anything like that.

Sirraide commented 5 months ago

It should be noted that including files is not supported when loading from a string, but my reasoning here is that that if you’re already providing sound changes inline, then you probably won’t want to use includes anyway; another reason is that it wouldn’t really be that clear what include paths should be relative to now that there is no file backing the data.

neta-elad commented 5 months ago

I think overall this is a good thing to support, but I fear this implementation is too unidiomatic... Take a look at https://github.com/Kotlin/kotlinx.serialization/blob/master/docs/polymorphism.md to see how this could be better handled by the built-in polymorphism support of kotlinx.serialization. This could also simplify the protocol. E.g., the load_string request type could be simplified to

{"type": "load_string", "data": "..."}

You mentioned you're not that familiar with Kotlin, so let me know if you need some help to refactor your code.

Also small nitpick - I would add a test to check (and describe) the response when #include directives are used inside of a load_string request.

Sirraide commented 5 months ago

I fear this implementation is too unidiomatic...

I’m not surprised, it did feel somewhat janky to me too. I’ll take a look at the document you linked.

Sirraide commented 5 months ago

Also small nitpick - I would add a test to check (and describe) the response when #include directives are used inside of a load_string request.

That is probably a good idea

Sirraide commented 5 months ago

Alright, I think I’ve managed to clean this up a bit. At least it looks a lot better to me now

Sirraide commented 5 months ago

Should all be done now. I’ve also added a test for that error.