search

ruma / synapse-admin-api

Ruma API declarations for the synapse admin API
MIT License
8 stars 5 forks source link

refactor: Move register responses into types #18

Closed stoically closed 2 years ago

stoically commented 2 years ago

Just a realization as consumer: This would not have been needed with serialize/deserialize directly on the ruma_api! response.

jplatte commented 2 years ago

This PR doesn't make explain at all why this change is being made. Can you please explain?

stoically commented 2 years ago

Because otherwise I can't serialize/deserialize the response. Which I thought I need, but actually is no longer needed if I use the ruma/sdk send methods, right.

Still, if I wanted to do de/serialization - like, storing the account as JSON somewhere, I would need the refactor again. Which would be solved by deriving on the response.

So, feel free to close if you prefer.

jplatte commented 2 years ago

If we were to allow request / response (de)serialization, it should be done consistently, not like this. It's something that's been discussed in Ruma before, but I'm not a fan of. I couldn't find the issue right now, but it seems like it's not so important right now anyways.

stoically commented 2 years ago

Yeah, makes sense. If you happen to stumble over the issue, I'd be interested in the link.

jplatte commented 2 years ago

I checked again, here it is: https://github.com/ruma/ruma/issues/10

It was closed because nobody showed interest in it and the implementation bits from the issue description went out of date. I think I would also push back harder against such a feature now should it be proposed again, since it would make it easier for people to do the wrong thing in serializing all request fields as body fields.