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

Added README to messages crate #993

Merged
merged 5 commits into from
Sep 25, 2023
Merged

Added README to messages crate #993

merged 5 commits into from
Sep 25, 2023

Conversation

bobozaur
Copy link
Contributor

@bobozaur bobozaur commented Sep 23, 2023

Adds a README to the messages crate to make it easier to understand the crate architecture/design and the ways to extend it.

@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2023

Codecov Report

Merging #993 (6d78fb0) into main (b71327e) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 6d78fb0 differs from pull request most recent head 2cc5bc4. Consider uploading reports for the commit 2cc5bc4 to get more accurate results

@@           Coverage Diff           @@
##             main     #993   +/-   ##
=======================================
  Coverage   30.02%   30.02%           
=======================================
  Files         415      415           
  Lines       26916    26916           
  Branches     5243     5243           
=======================================
+ Hits         8081     8082    +1     
  Misses      16639    16639           
+ Partials     2196     2195    -1     
Flag Coverage Δ
unittests-aries-vcx 30.02% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

# messages
The `messages` crate provides data structures representing the various messages used in Aries protocols. This `README` explores the architecture behind the crate and module organization, making implementing new messages/protocols easier.

### Message strucutre
Copy link
Contributor

Choose a reason for hiding this comment

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

nit "strucutre" -> "structure"

Copy link
Contributor

@gmulhearn-anonyome gmulhearn-anonyome 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 this. has helped my PR

Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
gmulhearn
gmulhearn previously approved these changes Sep 25, 2023

When deciding the minor version to use in a protocol, the protocol and version of the `@type` field is looked up in the `PROTOCOL_REGISTRY` (a lazily initialized map containing all protocols implemented). On success, the specified protocol version can be used as it is implemented. On failure, though, the minor version is decremented and looked up again until either the minor version reaches `0` or the lookup succeeds.

**NOTE:** The `Protocol` enum has values like: `Protocol::ConnectionType::ConnectionTypeV1::V1_0`. The exact message kind is not part of the enum but rather there is type-linking involved (hence the arrow ->). This allows the `Protocol` enum to represent only protocols and protocols version by itself, while also providing mechanisms for parsing the message kind and thus getting the exact message to further process.
Copy link
Contributor

Choose a reason for hiding this comment

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

The exact message kind ...

I think readme is a good place to clearly define the terms "message type" and "message kind"; it already does, but rather implicitly, and since this might be one of the first questions a reader of the crate might have, it might be helpful to make the explanation easy to find and clearly stated in the readme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point. Added a Glossary section.


- AriesMessage
- Connection
- ConnectionV1
Copy link
Contributor

Choose a reason for hiding this comment

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

When first reading this, it is not immediately obvious to me what the individual bullets represent, and what is the relationship between the parent and children, so perhaps that warrants further explanation (which is somewhat provided further, but it might be better to provide it before presenting this list).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair. I tweaked the preceding sentence a bit to kinda make it clearer. Let me know if it's better now.

@@ -0,0 +1,84 @@
# messages
The `messages` crate provides data structures representing the various messages used in Aries protocols. This `README` explores the architecture behind the crate and module organization, making implementing new messages/protocols easier.
Copy link
Contributor

Choose a reason for hiding this comment

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

... making implementing new messages/protocols easier.

Understanding the nuts and bolts of a crate of course makes extending it easier, however I wonder it is (or should be) necessary. Perhaps the easiest way to explain how to extend a crate which is designed to be easily extended (like this one) is a PR doing just that - as long as the process is generally systematic. Of course, this doesn't work when it isn't, and get's outdated when the crate's workings are changed substantially (which is a problem with documentation as well).

This is not meant to be actionable, just thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idk if a reference PR is a good or a bad thing. I never thought of it, to be honest. @Patrik-Stas @gmulhearn what do you think?

Nevertheless, I added an Extending the crate section trying to describe the main concepts/steps behind extending the crate. I don't know whether it helps so feedback is welcome.

I'd do the PR thing but I feel like down the line it will get outdated for some reason as we'll keep forgetting about it. And depending on whether you're just adding some message (like ProblemReport) to a protocol, adding a minor/major version or adding an entirely new protocol the implementation will differ a bit. So then you either have multiple PR's for each or one PR where you do all and it might actually end up being a bit confusing as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah as you both mentioned, there's the problem of getting outdated. I think adding Extending the crate is good enough.

messages/README.md Outdated Show resolved Hide resolved
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

All labels are on the same level ###, no biggie but consider some levelling, for example I can imagine some top level label Message Structure with 3 subsections under that

@Patrik-Stas Patrik-Stas merged commit c558252 into main Sep 25, 2023
38 of 40 checks passed
@Patrik-Stas Patrik-Stas deleted the update/messages_docs branch September 25, 2023 14:57
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.

6 participants