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 /r/parents/:inscription_id/inscriptions endpoint. #4088

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

gmart7t2
Copy link
Contributor

Analogous to #3771 but for parents instead of children.

@gmart7t2
Copy link
Contributor Author

The examples in the docs for /r/children/.../inscription* have the full output, so I did the same here.

Maybe it makes sense to shorten those examples, too?

Copy link
Collaborator

@casey casey left a comment

Choose a reason for hiding this comment

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

Looks good! The errors in parent_inscriptions_recursive should be handled, see my comment. In general, errors from the index shouldn't be ignored. unwraps on Options are okay though, when we expect the index to contain the inscription.

.map(|inscription_id| {
let entry = index
.get_inscription_entry(inscription_id)
.unwrap()
Copy link
Collaborator

Choose a reason for hiding this comment

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

These errors should be handled. If the result type of a map is Result<T>, then you can collect into a Result<Vec<T>>, which will return an error if any of the results is Result::Err.


let satpoint = index
.get_inscription_satpoint_by_id(inscription_id)
.ok()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, this error should be handled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready for Review
Development

Successfully merging this pull request may close these issues.

3 participants