-
Notifications
You must be signed in to change notification settings - Fork 8
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
Rework Frontend Trait #21
Comments
That sounds like an excellent idea to me. I haven't followed Xi development for a long time, so I bet
I'm not sure I understand: adding a new item to an enum is a breaking change, and we'll need to add/remove items as rpc methods are added/removed from xi-core no? Edit: sorry for taking such a long time to reply back. Btw I'll be AFK from tonight until May 10th or 11th. |
We could prevent it being a breaking change pretty easily actually if we do one thing. Suggest that users of this library match againts _ instead of all enum variants like this.
this way when a new enum variant is added people can upgrade the xrl crate but still only handle the variants they are currently using. So if a 'Variant::3' was added to the example enum above it would'nt be a breaking change to anyone who uses an underscore in there match expression. |
Currently as written the 'Frontend' trait has a method for each xi-event. This is Alright now but will get pretty unmanageable as xi adds more methods. I think an enum should be created that represents each xi-core response, we can call it XiEvent for now, that way 'Frontend' can only have one method that takes an instance of XiEvent. This would make the trait easier to implement, also would make adding features to the frontend easier without it being a breaking change everytime xi-core gets a new rpc method.
Since this would be a breaking change i'll wait a while before opening a PR so anyone who might be using the library might have time to see this.
@little-dude since this would be a breaking change i would like it if you could give your thoughts on this
The text was updated successfully, but these errors were encountered: