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 methods less internal #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

entropylost
Copy link
Contributor

This pull request makes quite a lot of __internal_ things on DomBuilder, StylesheetBuilder, ClassBuilder visible from outside.

Why do I want this?

  • Currently, the only way to use these methods are from utility macros, despite them being perfectly fine usable by themselves.
    For example, instead of just doing ClassBuilder::new().style("foo", "bar").finish(), I am forced to call the macro.
    Similarly, the with_node! macro only works within a html! context, and not a DomBuilder::new_html("foo")....into_dom()
  • Without the methods being public, its hard to make custom macros, as I have to directly inspect the source code.
  • As the macros to access the methods are currently undocumented, it is difficult to see which macro should be used. For example, from looking at the ClassBuilder documentation it is unclear how to generate one; this could also be solved by documenting the macros, but that might better be served by exposing the methods and saying "You can also use with_node!".

This pull request also adds a finish() method to DomBuilder, in order to make its api more similar to StylesheetBuilder and ClassBuilder, which both are given finish() methods.

@dakom
Copy link

dakom commented Sep 3, 2021

fwiw I hit a similar constraint when trying to add additional event macros: https://github.com/dakom/dominator-helpers/blob/e1ec52fe658dae70c28f1db10dd89ce47b80b607/src/events.rs#L4

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