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

Parse misuse resistance #191

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

sftse
Copy link
Contributor

@sftse sftse commented Dec 12, 2024

The parse function is potentially easy to misuse.
Also, some unrelated refactors.

…municate to later stages

The following code can easily be mistaken for using the context to
store information retrieved by render_to_lines. This is actually not
the case.

let mut context = self.make_context();
self.do_parse(&mut context, input)
      .render_with_context(&mut context, 100, self.decorator);
@sftse sftse force-pushed the parse-misuse-resistance branch from 2c3a1c2 to de29b06 Compare December 13, 2024 16:45
@jugglerchris
Copy link
Owner

Hi,
Thanks for another MR!
Could you please separate "unrelated refactors" and clippy changes to separate MRs if possible? I find it much harder to review changes with the other non-functional changes mixed in (the separate commits for parts of the change definitely help, but it's too easy to miss things in the whole diff).

Note that it does not hold that HtmlContext::default() == config::plain().make_context()
but the relevant fields used by fn parse_with_context() are the same.
This is a bit unclear, so this change makes this more transparent.
@sftse sftse mentioned this pull request Dec 16, 2024
@sftse sftse force-pushed the parse-misuse-resistance branch 2 times, most recently from a78b581 to 0ec601f Compare December 16, 2024 10:58
@sftse sftse marked this pull request as draft December 16, 2024 11:20
@sftse
Copy link
Contributor Author

sftse commented Dec 16, 2024

If #192 gets accepted, we'll rebase on main. What remains is the last commit, which originally suggested to replace pub fn parse with the method pub fn do_parse. We looked at this part of the code in preparation to an upcoming PR in which we'll request additional internal configuration to be exposed, either configuration of RenderTree or exposing the Renderer trait, but probably not both.

On second thought, we're not so sure anymore we'll want to configure RenderTree over using the Renderer trait, so we're pausing these suggestions until we have to cross that bridge. These changes are clearly not critical, since there is probably nobody using these functions.

…method on Config

The parse function is an odd one in the current API, other functions
can be separated cleanly into functions that are convenient and functions
that need to be configured. The parse function does not advertise what
configuration it uses, and will silently drop css information, as it parses
with config::plain() as default configuration. This can lead to the following
misuse of the public API

let render_tree = parse(html)?;
customcfg.render_to_string(render_tree, 80)?;
@sftse sftse force-pushed the parse-misuse-resistance branch 2 times, most recently from 1b9fc9c to fbaf8b3 Compare December 16, 2024 11:59
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.

2 participants