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

Read only Json API #2250

Merged
merged 16 commits into from
Aug 10, 2023
Merged

Read only Json API #2250

merged 16 commits into from
Aug 10, 2023

Conversation

Mathieu-Be
Copy link
Contributor

Opening a new PR based on the work of @ynohtna92 on #2036.

I'm not super experienced in Rust so I'm opening the PR to get early feedback, only the sat endpoint is implemented as of now.

I'm wondering if it is really necessary to have the exact same fields as the HTML response, is the _links section really necessary for example ?

Also, I feel like there could be a better way to build the Json response, but if I change the Serialize impl of Sat it is breaking other stuff, so not sure what to do here (maybe create a SatApiResponse struct based on Sat ?)

@ynohtna92
Copy link
Contributor

_links aka HATEOAS make the API crawlable, it is better to have them then not have them.

@raphjaph
Copy link
Collaborator

@Mathieu-Be just implemented the small changes I requested. Have a look if you agree with them.

@raphjaph raphjaph requested review from casey and veryordinally July 24, 2023 18:06
@raphjaph raphjaph mentioned this pull request Jul 25, 2023
@raphjaph
Copy link
Collaborator

raphjaph commented Jul 28, 2023

I'm going to leave out the _links for now but may add them later if I see a good use case.

Closes: #1662

@Mathieu-Be
Copy link
Contributor Author

Mathieu-Be commented Jul 31, 2023

@Mathieu-Be just implemented the small changes I requested. Have a look if you agree with them.

Cool ! I haven't seen the changes you requested, did they get lost somewhere ? The Json struct looks nicer indeed.

Want me to take a stab at the other endpoints ?

@raphjaph
Copy link
Collaborator

Cool ! I haven't seen the changes you requested, did they get lost somewhere ? The Json struct looks nicer indeed.

I reviewed them above. Maybe they are collapsed (grey box)?

Want me to take a stab at the other endpoints ?

That would be great! But before you start I think we should make the JSON endpoint configurable in this PR. Such that you have to pass a flag to activate it. Something like --with-json-api

@lifofifoX
Copy link
Collaborator

Before adding the /sat/N JSON endpoint, it might be worth getting some clarity on #2277 as it includes a proposal for the same path. While the server can return different responses based on the Accept header, it'd be nice to use different paths for returning JSON and inscriptions on the sat.

src/options.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@raphjaph raphjaph left a comment

Choose a reason for hiding this comment

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

Let's go!

@raphjaph raphjaph enabled auto-merge (squash) August 10, 2023 10:21
@raphjaph raphjaph merged commit 13d5c2f into ordinals:master Aug 10, 2023
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.

4 participants