Skip to content
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

Merged
merged 7 commits into from
Jul 12, 2023
Merged

Core elements added. #3

merged 7 commits into from
Jul 12, 2023

Conversation

BenediktBurger
Copy link
Member

@BenediktBurger BenediktBurger commented Jun 29, 2023

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 at protocols.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 a Communicator 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 omnipotent Message
  • serialization contains helper classes to (de)serialize data, could be included in message.
  • errors contains error objects (to check error ids, which are not yet defined)
  • test contains helper classes for tests, for example FakeSockets

P.S: Don't worry about the large line number: I have a lot tests and every file has their copyright notice...

Copy link
Member

@bilderbuchi bilderbuchi left a 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.

pyproject.toml Show resolved Hide resolved
pyleco/core/message.py Outdated Show resolved Hide resolved
pyleco/core/message.py Show resolved Hide resolved
pyleco/core/message.py Outdated Show resolved Hide resolved
pyleco/core/message.py Outdated Show resolved Hide resolved
pyleco/core/message.py Outdated Show resolved Hide resolved
pyleco/core/message.py Outdated Show resolved Hide resolved
pyleco/core/message.py Show resolved Hide resolved
pyleco/core/message.py Show resolved Hide resolved
pyleco/core/message.py Outdated Show resolved Hide resolved
@BenediktBurger
Copy link
Member Author

Thanks for your thorough review, @bilderbuchi

I see, that some confusion arose. Here is my concept regarding Message:

"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).
In order to do this data extraction only once (instead of twice for namespace and name), I use a cache, which stores the extracted values at first request.

Similarly, the data property (and constructor argument) is used for the content of the first payload frame (for example our json request).

Copy link
Member

@bilderbuchi bilderbuchi left a 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.

pyleco/core/protocols.py Outdated Show resolved Hide resolved
pyleco/core/protocols.py Outdated Show resolved Hide resolved
pyleco/core/protocols.py Outdated Show resolved Hide resolved
pyleco/core/protocols.py Outdated Show resolved Hide resolved
pyleco/core/protocols.py Outdated Show resolved Hide resolved
pyleco/core/serialization.py Outdated Show resolved Hide resolved
pyleco/core/serialization.py Outdated Show resolved Hide resolved
pyleco/errors.py Outdated Show resolved Hide resolved
pyleco/errors.py Outdated Show resolved Hide resolved
pyleco/test.py Show resolved Hide resolved
@BenediktBurger
Copy link
Member Author

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.

Benedikt Moneke and others added 2 commits July 1, 2023 19:16
Type hinting improved.
Comments added.
Changed to LECO lingo.
named tuples for header and names.
@BenediktBurger
Copy link
Member Author

BenediktBurger commented Jul 11, 2023

Now one question remains: should we remove sender_name and sender_node, as they are accessible via sender_elements.name and sender_elements.namespace?

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 sender_elements_str (restoring symmetry)

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.
@bilderbuchi
Copy link
Member

🤷 I don't have an opinion on this, do as you prefer please. :-)

@BenediktBurger
Copy link
Member Author

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 😁

@BenediktBurger
Copy link
Member Author

I removed all the string properties from Message. We can always add them later, if we deem it useful.

Copy link
Member

@bilderbuchi bilderbuchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@BenediktBurger BenediktBurger merged commit 1734a92 into main Jul 12, 2023
@BenediktBurger BenediktBurger deleted the add-core branch July 12, 2023 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants