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

Document vast majority of protocol messages #46

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

jcspencer
Copy link
Collaborator

This PR adds documentation for the structures in the proto module with references back to the agent protocol specification.

I'll come back later and add top-level documentation for these modules.

I've held off on documenting the agent module as it's undergoing a bit of renovation at the moment!

I've tried to say as close to the specification with this documentation, but have added some simplifications of the spec in some places, so very open to feedback / nits :^)

@jcspencer jcspencer force-pushed the jcspencer/documentation branch 2 times, most recently from c31470a to 1838cf7 Compare April 17, 2024 14:55
@wiktor-k
Copy link
Owner

Wow, really awesome job! 🎉

I'll read it carefully but it looks so good that it's hard to resist pressing "Merge pull request" right now! 😬

Copy link
Owner

@wiktor-k wiktor-k left a comment

Choose a reason for hiding this comment

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

Okay, this looks very good! A couple more of these doc comments and we'd be able to turn on #![deny(missing_docs)] 😅

I think the plan forward would be to get #48 merged first, then this PR may be rebased on top of main, adjusting for the changes and then we merge this.

Thank you!

src/proto/message.rs Outdated Show resolved Hide resolved
src/proto/extension.rs Outdated Show resolved Hide resolved
@jcspencer
Copy link
Collaborator Author

Thanks for the review @wiktor-k! I'll wait for #48 and rebase this on top :^)

@wiktor-k
Copy link
Owner

Okay, I see just minor formatting things being wrong here so if you don't mind I'll fix it and merge this PR... if there's anything to adjust later we can do it in a subsequent PRs :)

@wiktor-k wiktor-k force-pushed the jcspencer/documentation branch from 25fc116 to 908ff84 Compare April 19, 2024 07:19
@wiktor-k wiktor-k enabled auto-merge April 19, 2024 07:19
@wiktor-k wiktor-k merged commit 885e8e0 into main Apr 19, 2024
18 checks passed
@wiktor-k wiktor-k deleted the jcspencer/documentation branch April 19, 2024 07:24
@jcspencer
Copy link
Collaborator Author

Thanks!

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.

2 participants