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

Add DBus::Data, test and refactor PacketMarshaller, PacketUnmarshaller #103

Merged
merged 31 commits into from
Apr 4, 2022

Conversation

mvidner
Copy link
Owner

@mvidner mvidner commented Mar 23, 2022

This is a fix for #97.

To understand this, you should skim the Type System section of the D-Bus specification, up to and including the first table.

TODOs

1. DBus::Data

The design principle is to allow passing around such data which carries with it its exact D-Bus type: a 42 should know if it's a "y" BYTE or an "i" INT32. A ["foo", "bar"] should distinguish an "as" ARRAY of STRING from "(ss)" STRUCT of 2 STRINGs.

The concrete need for this was found when implementing org.freedesktop.DBus.Properties.GetAll whose return value is formally a dictionary of variants but those variants must each have a specific type of the individual property.

The biggest change is adding the DBus::Data module for exact representation of the D-Bus data types: Data::Int16, Data::UInt64, Data::Variant, Data::Array and so on.

So far the data entering and leaving dbus_methods are plain Ruby (Integer, Hash, ...) and DBus::Data are used internally.

2. (Un)Marshalling Reimplementation and Tests

DBus::Data is closely related to marshalling and unmarshalling, so PacketMarshaller and PacketUnmarshaller are largely reimplemented (splitting off RawMessage as a detail).

The test coverage for these classes was incomplete, so a comprehensive set of tests is added, using a data driven approach serialized in spec/data/marshall.yaml

@coveralls
Copy link

coveralls commented Mar 29, 2022

Coverage Status

Coverage increased (+0.1%) to 86.508% when pulling ecb5644 on data-unmarshal into d779e6c on master.

@mvidner mvidner marked this pull request as ready for review March 31, 2022 14:17
#
# A value is either {Basic} or a {Container}.
# {Basic} values are either {Fixed}-size or {StringLike}.
class Base
Copy link

Choose a reason for hiding this comment

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

Just a general reflection, I tend to distrust deep hierarchies specially when they contain artificial intermediate classes like StringLike.

In this case, I can see it makes sense in the way it is. I just wanted to make a note that this activated my arachnid sense.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good point, the intermediate classes are either completely implementation detail (Int) or a direct transfer from the specification (Fixed, StringLike) and not really helpful for the user.

Would it help if their docs said "Abstract helper"?

Also, (module)Data::(class)Base could be eliminated and merged into class Data, similar to what I did in #104 with Type::Type.

A careful look is needed to determine if it can be removed.

Can a bad case occur? Bad case: an inner method mutates *buffer* and
then IncompleteBufferException occurs, losing the partialy unmarshalled data.
@mvidner
Copy link
Owner Author

mvidner commented Apr 4, 2022

The Container types are still of Beta quality. I will make this a beta gem and be ready to fix bugs fast.

@mvidner mvidner merged commit 6cdd910 into master Apr 4, 2022
@mvidner mvidner deleted the data-unmarshal branch April 4, 2022 08:01
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.

3 participants