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

Port net/json-rpc from clan to std #826

Merged
merged 12 commits into from
Sep 16, 2023
Merged

Port net/json-rpc from clan to std #826

merged 12 commits into from
Sep 16, 2023

Conversation

fare
Copy link
Collaborator

@fare fare commented Sep 12, 2023 •
edited by vyzo
Loading

Adds support for JSON-RPC to the stdlib, both client and server side.

Fixes #815

  • Code
  • Tests
  • Docs

@fare fare requested a review from vyzo September 12, 2023 17:56
vyzo
vyzo requested changes Sep 12, 2023
View reviewed changes
Copy link
Collaborator

@vyzo vyzo 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 so far, but there are a couple of things that stick out.

.gitignore Outdated Show resolved Hide resolved
src/std/error.ss Show resolved Hide resolved
src/std/net/json-rpc.ss Outdated Show resolved Hide resolved
src/std/net/json-rpc.ss Outdated Show resolved Hide resolved
@vyzo vyzo requested a review from belmarca September 12, 2023 18:01
@vyzo
Copy link
Collaborator

vyzo commented Sep 13, 2023

build fails.

vyzo
vyzo reviewed Sep 13, 2023
View reviewed changes
src/std/net/httpd/handler.ss Outdated Show resolved Hide resolved
src/std/net/httpd/handler.ss Outdated Show resolved Hide resolved
src/std/net/json-rpc-test.ss Show resolved Hide resolved
src/std/net/json-rpc.ss Outdated Show resolved Hide resolved
src/std/stxutil.ss Outdated Show resolved Hide resolved
src/std/net/json-rpc.ss Outdated Show resolved Hide resolved
src/std/net/json-rpc.ss Outdated Show resolved Hide resolved
src/std/net/json-rpc.ss Outdated Show resolved Hide resolved
src/std/net/json-rpc.ss Show resolved Hide resolved
@fare fare force-pushed the json-rpc branch 3 times, most recently from 645f8a6 to df03cc6 Compare September 13, 2023 16:35
vyzo
vyzo reviewed Sep 13, 2023
View reviewed changes
Copy link
Collaborator

@vyzo vyzo 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 overall, but I have left comments that I would like to address (or create follow up issues to address later).

Also, can you rebase on master and adjust for SSL? We need to pass ssl contexts to the requests.

src/std/misc/list.ss Show resolved Hide resolved
src/std/misc/process.ss Outdated Show resolved Hide resolved
src/std/misc/string.ss Outdated Show resolved Hide resolved
src/std/misc/with-id-test.ss Outdated Show resolved Hide resolved
src/std/misc/string.ss Outdated Show resolved Hide resolved
src/std/net/json-rpc.ss Outdated Show resolved Hide resolved
src/std/net/json-rpc.ss Outdated Show resolved Hide resolved
src/std/net/json-rpc.ss Outdated Show resolved Hide resolved
src/std/stxutil.ss Outdated Show resolved Hide resolved
src/std/text/json/util.ss Outdated Show resolved Hide resolved
@vyzo vyzo mentioned this pull request Sep 14, 2023
Open
@fare fare force-pushed the json-rpc branch 5 times, most recently from 9441085 to 822375a Compare September 15, 2023 02:42
@fare
Copy link
Collaborator Author

fare commented Sep 15, 2023

Please take another look.

vyzo
vyzo reviewed Sep 15, 2023
View reviewed changes
Copy link
Collaborator

@vyzo vyzo 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.

This looks pretty good and polished, thank you.

Can we resolve the string-substitute-char argument order situation?
Also, mutex for the global counter.
Other than that some nits that you may choose to ignore.

Once we've resolved open conversations, we can merge.

src/std/misc/process.ss Show resolved Hide resolved
src/std/misc/string.ss Outdated Show resolved Hide resolved
src/std/misc/with-id-test.ss Outdated Show resolved Hide resolved
src/std/net/json-rpc.ss Outdated Show resolved Hide resolved
src/std/net/json-rpc.ss Outdated Show resolved Hide resolved
src/std/net/json-rpc.ss Outdated Show resolved Hide resolved
src/std/net/json-rpc.ss Outdated Show resolved Hide resolved
src/std/net/json-rpc.ss Outdated Show resolved Hide resolved
src/std/net/uri.ss Outdated Show resolved Hide resolved
@fare
Copy link
Collaborator Author

fare commented Sep 15, 2023

Should I split the file in 3?

  • client
  • server (transport-independent)
  • httpd handler

@vyzo
Copy link
Collaborator

vyzo commented Sep 15, 2023

Up to you.

If you do, add a fourth api file in the package which exports the public api, and make all the others export #t.
Then create a module std/net/json-rpc that reexports api.

@fare fare force-pushed the json-rpc branch 3 times, most recently from a12e82a to edae228 Compare September 16, 2023 03:23
vyzo
vyzo reviewed Sep 16, 2023
View reviewed changes
Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

Ok, it is good.

The only objection i have is this junk-allowed business in read-json, which i think we should remove.
The json reader has no business reading past the delimiter.

I will not touch, but I advise that we remove it in a follow up.

I will do a quick optimization pass in some slow utility code, and then merge.

src/std/misc/atom.ss Outdated Show resolved Hide resolved
src/std/misc/atom.ss Outdated Show resolved Hide resolved
src/std/misc/atom.ss Outdated Show resolved Hide resolved
src/std/misc/atom.ss Outdated Show resolved Hide resolved
src/std/misc/process.ss Show resolved Hide resolved
src/std/text/json/input.ss Outdated Show resolved Hide resolved
src/std/values.ss
list->values ;; NB: values->list is builtin
values->cons cons->values)

(defrules first-value ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can be optimized.

src/std/values.ss Show resolved Hide resolved
src/std/values.ss Show resolved Hide resolved
src/std/values.ss Show resolved Hide resolved
@vyzo
Copy link
Collaborator

vyzo commented Sep 16, 2023

I will not touch, but I advise that we remove it in a follow up.

Actually, I will. It vexes me.

@netlify
Copy link

netlify bot commented Sep 16, 2023 •
edited
Loading

✅ Deploy Preview for elastic-ritchie-8f47f9 ready!

Name Link
🔨 Latest commit d57e553
🔍 Latest deploy log https://app.netlify.com/sites/elastic-ritchie-8f47f9/deploys/650593e85d463700081349c5
😎 Deploy Preview https://deploy-preview-826--elastic-ritchie-8f47f9.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@vyzo
Copy link
Collaborator

vyzo commented Sep 16, 2023

rebased on master in preparation for merge.

vyzo
vyzo approved these changes Sep 16, 2023
View reviewed changes
Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

ok, this is ready now.

@vyzo vyzo merged commit 918fa61 into master Sep 16, 2023
@vyzo vyzo deleted the json-rpc branch September 16, 2023 12:44
@vyzo vyzo added this to the Gerbil v0.18: Nimzowitsch milestone Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vyzo vyzo vyzo approved these changes

@belmarca belmarca Awaiting requested review from belmarca

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Gerbil v0.18: Nimzowitsch
Development

Successfully merging this pull request may close these issues.

JSON-RPC Support in stdlib
2 participants
@fare @vyzo