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 it possible to pass context and encoding arguments from external sources #76

Open
dmbaturin opened this issue Sep 8, 2023 · 1 comment

Comments

@dmbaturin
Copy link

dmbaturin commented Sep 8, 2023

The type of Markup.parse_html is ?report:(location -> Error.t -> unit) -> ?encoding:Encoding.t -> ?context:[< `Document | `Fragment of string ] -> (char, 's) stream -> 's parser. That is quite handy in that you can call it without context and encoding arguments.

If you are writing a utility of a library that calls Markup.parse_html, that can become a source of annoynance. With encoding it's easy: the default is Markup.Encoding.utf_8, so you can easily do something like:

Markup.parse_html ~encoding:(Option.value ~default:Markup.Encoding.utf_8 settings.encoding

The option wrap-unwrap cycle is rather inelegant but at least there's no code duplication needed.

However, with the context argument, there is no way to achieve the same effect in one function call! The value that parse_html takes can be either `Document or `Fragment "tag" — two-state. There is no polymorphic variant constructor that would mean "guess the context", like Html_parser.parse None ... does.

So, the only way to pass the context from an external source is to use two different calls, one for "context not given, guess", the other for explicit context.

match settings.context with
| None -> ... |> Markup.parse_html ~encoding:settings.encoding |> ...
| Some c -> ... |> Markup.parse_html ~context:c ~encoding:settings.encoding |> ...

I'm not sure what's the best way out. I see three options:

  • Change the type of Markup.parse_html to ~context:(< Document | Fragment of string> option) — that would be a breaking change.
  • Add a new polymorphic variant constructor like `Any — non-breaking but makes the internal logic quite inelegant.
  • Add a new function with a different name — compatible but sacrifices the API elegance.
@dmbaturin
Copy link
Author

dmbaturin commented Sep 8, 2023

Ok, I completely missed the fact that you can set an implicit optional argument explicitly with ?context:settings.context.

But I still believe there should be an explicit polymorphic variant for the default context.

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

No branches or pull requests

1 participant