-
Notifications
You must be signed in to change notification settings - Fork 3
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
Core elements added. #3
Conversation
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.
Thanks for dividing things up! I looked through Message
at first, I hope my feedback is useful!
Will have to continue with protocols later.
Thanks for your thorough review, @bilderbuchi I see, that some confusion arose. Here is my concept regarding "real" attributes (not via property) are the frames defined by LECO. Access to the information in the frames (e.g. namespace and name in the full_name of a sender or receiver) is done via properties (read_only). Similarly, the |
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.
Second part, doen with the first pass.
Thanks for your extensive review. The protocol definitions (except for the Communicator) will enter the LECO protocol as "message types" (pymeasure/leco-protocol#29), so the two are closely related. I have tests in place, which verify, that the Components follow the Protocols, so it is very helpful to define the Protocols first. |
Type hinting improved. Comments added. Changed to LECO lingo. named tuples for header and names.
Now one question remains: should we remove I'd say yes (less code to maintain) On the other hand, especially the conversation_id is needed many times (for responses), such that a "message.conversation_id" remains handy. Similarly, the string versions can be reduced to |
Protocols separated into leco ones and internal (to pyleco) ones. Protocols carry `Protocol` in their name. Message includes more checks and does not include cached values anymore. RPCGenerator simplified.
🤷 I don't have an opinion on this, do as you prefer please. :-) |
I removed the name related entries (and their string counterparts). We can always add them later again, but removing them is difficult, once they have been added. I'll remove also the header parts, except for the conversation_id, which is important (to know which message is the answer to a certain request). I already did write (and test) the code. Tomorrow I'll push it. WIth that, we will have the first part of pyleco 😁 |
I removed all the string properties from |
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.
LGTM, thanks!
In this PR I add essential elements for further pyleco modules.
I ask you to look at
Message
, which is a real important for all the other things, and atprotocols.py
which contains the API definitions (which I will propose in LECO as commands, different Components have to offer).Content:
rpc_generator
contains a helper class to generate json rpc calls and to interpret the response.protocols
contains class stubs defining what a Component has to offer, or what aCommunicator
has to offer. Communicator is a pyleco tool, which a software (e.g. a GUI) may use to send and receive LECO messages. For example the Directors require a Communicator.message
contains the omnipotentMessage
serialization
contains helper classes to (de)serialize data, could be included inmessage
.errors
contains error objects (to check error ids, which are not yet defined)test
contains helper classes for tests, for example FakeSocketsP.S: Don't worry about the large line number: I have a lot tests and every file has their copyright notice...