fix: update server version endpoint with optional python version #23
Conversation
hanadi92
force-pushed
the
update-server-version-api
branch
2 times, most recently
from
5d0bb7c to
deb14a8
Compare
Signed-off-by: Hanadi92 <hanadi.tamimi@gmail.com>
hanadi92
force-pushed
the
update-server-version-api
branch
from
deb14a8 to
bf5954b
Compare
src/version/get_server_version/v1.rs
Outdated
| @@ -33,7 +37,51 @@ impl Request { | |||
|
|
|||
| impl Response { | |||
| /// Creates a `Response` with the given Synapse and Python versions. | |||
| pub fn new(server_version: String, python_version: String) -> Self { | |||
| pub fn new(server_version: String, python_version: Option<String>) -> Self { | |||
There was a problem hiding this comment.
Could you just remove the python_version parameter instead? I don't think a Rust server implementation usually has a reason to set this field, and it can still be set to Some via direct field access after creating the response with Response::new.
It's a breaking change either way.
There was a problem hiding this comment.
Do you mean something like this?
pub fn new(server_version: String) -> Self {
Self { server_version, python_version: None }
}Or completely removing the python_version from the Response struct (which would break old versions no?)
There was a problem hiding this comment.
Yes, I mean exactly what you put in the code snippet. Removing the fields wouldn't break clients that deserialize this response and only check the synapse version field, but I think it's slightly more useful to keep the field, but remove it from the constructor.
There was a problem hiding this comment.
Done ✅ Thank you so much for your review!
Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
Signed-off-by: Hanadi92 <hanadi.tamimi@gmail.com>
Signed-off-by: Hanadi92 <hanadi.tamimi@gmail.com>
As mentioned in the docs of admin api, the python_version is no longer part of the response since 1.94.0.
Signed off: @hanadi92