-
Notifications
You must be signed in to change notification settings - Fork 77
Refine Repo README and payjoin lib.rs #1248
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
Conversation
38e7626 to
222518b
Compare
Pull Request Test Coverage Report for Build 20638228938Details
💛 - Coveralls |
DanGould
left a comment
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.
Epic much needed changes to our docs. Where I made suggestions about specific wording, I tried to offer a suggestion that you can accept by simply clicking. My main concern is the addition of a payjoin/readme including examples which can't be checked against the compiler. I think it's more rusty to leave the top-level readme as the payjoin crate readme, as a sort of entrypoint into the project. Then, once someone opens docs.rs they can see the payjoin module's lib.rs rustdocs that are checked by the compiler for examples and details.
Once those are checked I think this is ready to go.
| ### [`ohttp-relay`](https://github.com/payjoin/rust-payjoin/tree/master/ohttp-relay) | ||
|
|
||
| ### `payjoin-ffi` | ||
| A Rust implementation of an Oblivious HTTP (OHTTP) relay resource. |
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.
It's worth mentioning high-level bip77 specific changes here and definitely in the ohttp-relay readme. TODO
Mirror the payjoin crate README in rustdocs
222518b to
1032df9
Compare
|
@DanGould Thanks for all the feedback. I addressed all of them. After seeing the rust-bitcoin and rust-lightning references you shared, I decided to keep the lib.rs content after the change minimal, with a very short summary of the crate and the features available. Otherwise, all of the other changes are going to be similar to what you first reviewed, plus the changes you recommended. |
DanGould
left a comment
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.
ACK 1032df9
Thanks for the swift turnaround. This takes us much closer to legible documentation. Thanks also for identifying the problem and suggesting solutions on your own volition.
I noticed the image won't render but don't think it's a big idea. IDK if any images render on docs.rs or using docsrs at all.
| #![cfg_attr( | ||
| docsrs, | ||
| doc( | ||
| html_logo_url = "https://raw.githubusercontent.com/payjoin/rust-payjoin/master/static/monad.svg" | ||
| ) | ||
| )] |
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.
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.
I expect the logo to be on the left Table of Contents panel (BDK reference here). I did not see it render when I generated the documentation locally, but I did not give it too much thought after reading somewhere how the local build did not have external content (image, etc.). If it does not render with the new release, I'll remember to come back to this and see what's happening.
| - Dart | ||
| - Javascript | ||
| - Python |
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.
As these are published, we may link them

Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.
Overview
This PR contains a couple of changes related to the higher-level documentation in the repository:
Closes #1216