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

Pass arguments to parser modules #446

Merged
merged 2 commits into from
Feb 14, 2023

Conversation

Kuret
Copy link
Contributor

@Kuret Kuret commented Feb 13, 2023

Hi there!

For testing partials with fast_html, we needed to pass arguments to its parse method.
Since Floki did not support passing arguments to their HTML Parsers an option to pass arguments to the parser module was added.

Note that this only has effect on fast_html, since both mochiweb and html5ever do not support additional arguments.

Why we needed this option:
This was needed because (for example) we had a partial for a table row. When testing this partial on it's own, the output would ignore the tr and td element, since it was 'invalid html' as the parent table element was missing.

This can be solved by passing a context: "table" to fast_html's parse function, only Floki did not support passing along arguments.

For example:

Floki.parse_fragment("<tr><td>Column 1</td><td>Column 2</td></tr>")
> {:ok. ["Column 1Column2"]}

Floki.parse_fragment("<tr><td>Column 1</td><td>Column 2</td></tr>", parser_args: [context: "table"])
> {:ok. [{ "tbody", [], [{"tr", [], [{"td", [], ["1"]}, {"td", [], ["2"]}]}]}]}

@philss
Copy link
Owner

philss commented Feb 14, 2023

Hi @Kuret 👋

Thank you for the PR. I liked the idea, but I think we could have a different API.

It's common in Elixir that when we want to pass options to a dependency that is configurable, we use the MFA or the MA approach. MFA stands for "Module, function and arguments", and is generally presented like:

{ModuleName, :function, [args]}

My idea is to accept the parser configuration in the MA way: {ModuleName, [parse_args]}.
So you would instead define an application config or use your function like this:

Floki.parse_fragment("<tr><td>Column 1</td><td>Column 2</td></tr>", parser: {CustomParser, [context: "table"]})

WDYT?

@Kuret
Copy link
Contributor Author

Kuret commented Feb 14, 2023

Hi,

Thanks for your reply.

Sounds good and is probably a more ‘Elixir’ way to implement it indeed.

@Kuret
Copy link
Contributor Author

Kuret commented Feb 14, 2023

When giving it another thought, it does sound like it’s easier to just pass the options when needed instead of passing the Module (which I already configured globally probably) with options every time a simple option is needed. You’re probably never going to use more than one (globally configured) module at once anyways.

Also, some of these options are function specific and only work on ‘fragment’ for example, and now it looks like the options are passed on the Module level.

Seems a bit redundant just to do it in a more ‘Elixir’ way?

@philss
Copy link
Owner

philss commented Feb 14, 2023

@Kuret makes sense. I think we can support both ways, and maybe merge the args if they are passed both as MA and :parser_args.

Thank you! 💜

@philss philss merged commit f3499c6 into philss:main Feb 14, 2023
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