-
Notifications
You must be signed in to change notification settings - Fork 50
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
Remove CyclerOutput from Communication Client and changes the communication protocol to only use the String
type for paths
#709
base: main
Are you sure you want to change the base?
Conversation
String
type for paths
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in chat, this PR breaks some architecture principles in type-safety of the communication protocol. The separation of output variant and path was intentional because we would otherwise mix concepts that don't belong together. Free string fields in protocols are usually a design smell which this PR violates. Please don't ignore Chesterton's fence.
PR #710 is now abusing this violation. As mentioned in chat, execution times have nothing to do with the data which cyclers produce and exchange. Timing information is orthogonal and should be exposed via a separate communication service. This allows further extension (e.g. profiling and more timing measurements) be made easily which is not possible with your proposed solution. Otherwise we repeat the architectural mistakes we currently have in the behavior simulator.
That's why I started the discussion publicly here 👍 |
@okiwi6, @h3ndrk and @knoellle discussed this today. |
Introduced Changes
Removes the
CyclerOutput
from the communication client. This means we can add features on the server side without modifying the client.Fixes #
ToDo / Known Issues
If this is a WIP describe which problems are to be fixed.
Ideas for Next Iterations (Not This PR)
If there are some improvements that could be done in a next iteration, describe them here.
How to Test