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.

Sign up for GitHub

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

Jump to bottom

implemented JSON formatting for Rune responses #2941

Merged
merged 12 commits into from
Jan 4, 2024
Merged

implemented JSON formatting for Rune responses #2941

merged 12 commits into from
Jan 4, 2024

Conversation

lugondev
Copy link
Contributor

@lugondev lugondev commented Jan 3, 2024

In this update, JSON formatting has been added to Rune responses to enhance data readability and interoperability.

thanks

@lugondev
Copy link
Contributor Author

lugondev commented Jan 3, 2024

Updated the Runes list response to return in JSON format. Thanks in advance for reviewing and merging.

raphjaph
raphjaph requested changes Jan 4, 2024
View reviewed changes
Copy link
Collaborator

@raphjaph raphjaph left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Good start! Left some comments, also try to use our formatter (just fmt) if you have just installed and maybe write a test in tests/json_api.rs for these two endpoints.

src/subcommand/server.rs Outdated Show resolved Hide resolved
src/subcommand/server.rs Outdated Show resolved Hide resolved
src/templates/rune.rs Outdated Show resolved Hide resolved
src/index.rs Outdated Show resolved Hide resolved
@lugondev lugondev closed this Jan 4, 2024
@raphjaph
Copy link
Collaborator

raphjaph commented Jan 4, 2024 •
edited
Loading

Why did you close this?

If you're having trouble I can just integrate my suggestions

@lugondev lugondev reopened this Jan 4, 2024
@lugondev
Copy link
Contributor Author

lugondev commented Jan 4, 2024

@raphjaph please review again my pull request

@lugondev
Copy link
Contributor Author

lugondev commented Jan 4, 2024

Why did you close this?

If you're having trouble I can just integrate my suggestions

haha, That was a mistake of mine :D

raphjaph
raphjaph requested changes Jan 4, 2024
View reviewed changes
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.

Looking good, two tests for the endpoints would also be good.

Can write those for you if you don't know where/how

src/subcommand/server.rs Outdated Show resolved Hide resolved
@lugondev @raphjaph
Update src/subcommand/server.rs
bf23db3 
Refactor the code for better simplicity

Co-authored-by: raph <raphjaph@protonmail.com>
@lugondev
Copy link
Contributor Author

lugondev commented Jan 4, 2024

Do you have any further suggestions for me?

@raphjaph
Copy link
Collaborator

raphjaph commented Jan 4, 2024

@lugondev I added some tests.

Still have to fix one thing but, have a look!

raphjaph
raphjaph approved these changes Jan 4, 2024
View reviewed changes
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.

It's good to go now

casey
casey requested changes Jan 4, 2024
View reviewed changes
tests/json_api.rs Outdated Show resolved Hide resolved
tests/test_server.rs Outdated Show resolved Hide resolved
@raphjaph raphjaph merged commit 16aa7ef into ordinals:master Jan 4, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@casey casey casey approved these changes

@raphjaph raphjaph raphjaph approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lugondev @raphjaph @casey