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

In-Code Documentation #226

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

antonengelhardt
Copy link

@antonengelhardt antonengelhardt commented Feb 27, 2024

In-Code Documentation for the following functions:

  • dispatch_http_call()
  • on_http_call_response()
  • on_vm_start()
  • get_vm_configuration()
  • on_configure()
  • get_plugin_configuration()
  • set_tick_period()
  • on_tick()
  • on_http_request_headers()
  • get_http_request_headers()
  • get_http_request_header()
  • add_http_request_header()
  • resume_http_context()
  • send_http_response()

Inspired by proxy-wasm/spec#42

Why?

To have documentation in your editor:

image

Copy link

google-cla bot commented Feb 27, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

* `dispatch_http_call()`
* `on_http_call_response()``
* `on_vm_start()`
* `get_vm_configuration()`
* `on_configure()`
* `get_plugin_configuration()`
* `set_tick_period()`
* `on_tick()`
* `on_http_request_headers()`
* `get_http_request_headers()`
* `get_http_request_header()`
* `add_http_request_header()`
* `resume_http_context()`
* `send_http_response()`

Signed-off-by: Anton Engelhardt <antoncengelhardt@icloud.com>
Signed-off-by: Anton Engelhardt <antoncengelhardt@icloud.com>
@antonengelhardt
Copy link
Author

antonengelhardt commented Feb 27, 2024

All examples do not include the RootContext implementation or main! Macro, as they are pretty similar in every case. So copy and pasting the examples, won't work, but I can still add them, if you think it's better.

@antonengelhardt
Copy link
Author

@PiotrSikora What do you think? Sorry to ping you, but I think this would be a great benefit for DX

Copy link
Contributor

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

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

@PiotrSikora What do you think? Sorry to ping you, but I think this would be a great benefit for DX

I agree those would be very helpful. Sorry for the delay, I'll review this in the coming days.

src/traits.rs Outdated
///
/// impl HttpContext for MyHttpContext {
/// fn on_http_request_headers(&mut self, _num_headers: usize, _end_of_stream: bool) -> Action {
/// let auth = self.get_http_request_header("Authorization").unwrap_or_defauklt();
Copy link
Contributor

Choose a reason for hiding this comment

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

defauklt -> default, which means that those examples are not tested/build in the doc publishing process.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry about that. I was a little careful when copy and pasting stuff, because it always messed up the doc formatting. But i have tested it ;)

src/traits.rs Show resolved Hide resolved
… stuff

Signed-off-by: Anton Engelhardt <antoncengelhardt@icloud.com>
@antonengelhardt
Copy link
Author

@PiotrSikora What do you think? Sorry to ping you, but I think this would be a great benefit for DX

I agree those would be very helpful. Sorry for the delay, I'll review this in the coming days.

@PiotrSikora I tested all documented functions again and set up a repo with 7 folders that cover all functions. It might still be that something is missing. I will do some diffs on the docs and the testing-repo-files to see if anything is missing apart from the boilerplate stuff (proxy_wasm::main!, RootContext Impl, etc.).

I hope this makes the review as easy as possible.

Signed-off-by: Anton Engelhardt <antoncengelhardt@icloud.com>
Signed-off-by: Anton Engelhardt <antoncengelhardt@icloud.com>
@antonengelhardt
Copy link
Author

I can squash my changes into one commit, if required.

@antonengelhardt
Copy link
Author

@PiotrSikora I can also add the examples from the mentioned repo to the examples/ directory...

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.

None yet

2 participants