Skip to content
This repository was archived by the owner on Jan 6, 2025. It is now read-only.

Expose lsps0_message_handler.handle_request#15

Closed
yanganto wants to merge 2 commits into
lightningdevkit:mainfrom
kuutamolabs:expose-handler-request
Closed

Expose lsps0_message_handler.handle_request#15
yanganto wants to merge 2 commits into
lightningdevkit:mainfrom
kuutamolabs:expose-handler-request

Conversation

@yanganto
Copy link
Copy Markdown
Contributor

Would lsps0_message_handler.handle_request be public and return the response?
Such that people integrate the handler will easier to verify the handler work after integration.
Thanks in advance. 🙏

@yanganto yanganto changed the title Expose handler request Expose lsps0_message_handler.handle_request Jul 27, 2023
@yanganto yanganto force-pushed the expose-handler-request branch from 58ba8f2 to 111ab58 Compare July 27, 2023 12:40
Copy link
Copy Markdown
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

I don't think we should expose arbitrary internals through the public API. So if you'll need access for testing I'd suggest to either change it locally or hide the exposure behind the cfg(test) flag.

Comment thread src/transport/msgs.rs
@@ -1,3 +1,4 @@
#![allow(missing_docs)]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We set #![deny(missing_docs)] crate-wide for good reason and def. should allow them here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is there demo client to request on the public interface of lsp handler? It will be nice for people to check after integrate the handler. Many thanks.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think @johncantrell97 has a branch of ldk-sample with the LSP client and LSPS2 integrated.

@johncantrell97, mind sharing a link here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I forget the state it's in but I think it was working: https://github.com/johncantrell97/ldk-sample/pull/1/files

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants