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

Make ElementContentHandlers and DocumentContentHandlers fields public. #194

Merged
merged 3 commits into from
Sep 13, 2023

Conversation

orium
Copy link
Member

@orium orium commented Sep 10, 2023

This is helpful if you want to patch already created handlers with extra functionality. For instance, if you want to measure the time a handler takes to run you can do something like:

fn measure_element_time_handler(handlers: ElementContentHandlers<'_>) -> = ElementContentHandlers<'_> {
   let element_handler = self.element.take().map(|handler| {
       |element| {
           // measure start time here.
           let result = handler(element);
           // measure end time here.
           result
       }
   });

   ElementContentHandlers {
       element: element_handler,
       comments: self.comments.take(),
       text: self.text.take(),
   }
}

@@ -5,7 +5,7 @@ use super::AsciiCompatibleEncoding;
use std::borrow::Cow;
use std::error::Error;

pub(crate) type HandlerResult = Result<(), Box<dyn Error + Send + Sync>>;
pub type HandlerResult = Result<(), Box<dyn Error + Send + Sync>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

We need doc comments for those and all the newly exposed API. Let's add #![warn(missing_docs)] to the crate's lib.rs and also use `RUSTFLAGS="-D warnings" to the CI builds (if we don't have it already) to fail on missing docs

Copy link
Member Author

@orium orium Sep 11, 2023

Choose a reason for hiding this comment

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

Will add documentation to those fields. However, we can't fail on missing documentation right because it is missing from a lot of places:

warning: `lol_html` (lib) generated 21 warnings

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's revisit those and suppress the warning where it's intentional or provide docs otherwise.

…blic.

This is helpful if you want to patch already created handlers with extra
functionality. For instance, if you want to measure the time a handler takes
to run you can do something like:

```
fn measure_element_time_handler(handlers: ElementContentHandlers<'_>) -> = ElementContentHandlers<'_> {
   let element_handler = self.element.take().map(|handler| {
       |element| {
           // measure start time here.
           let result = handler(element);
           // measure end time here.
           result
       }
   });

   ElementContentHandlers {
       element: element_handler,
       comments: self.comments.take(),
       text: self.text.take(),
   }
}
```
@inikulin inikulin merged commit cf82f21 into cloudflare:master Sep 13, 2023
4 checks passed
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.

3 participants