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.

Sign up for GitHub

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

Jump to bottom

Emit Unsafe.string_attrib for _-prefixed attributes in syntax ppxs to support non-standard attribute names #296

Merged
merged 2 commits into from
Jun 21, 2022
Merged

Emit Unsafe.string_attrib for _-prefixed attributes in syntax ppxs to support non-standard attribute names #296

merged 2 commits into from
Jun 21, 2022

Conversation

cemerick
Copy link
Contributor

@cemerick cemerick commented Jan 12, 2022 •
edited
Loading

Fixes #295

(also includes more generalized kebab-casing for attributes from jsx)

@cemerick
Copy link
Contributor Author

FWIW, I've been using this in anger for a week or so, and it's working out quite nicely to emit htmx-flavoured HTML.

Any thoughts on the approach?

@Drup
Copy link
Member

Drup commented Feb 8, 2022

Sorry about the late response.

Interesting escape hatch. Why do you need a_unchecked instead of just Unsafe.string_attrib ?

Since re is already in the dependency cone, if your patch gets in, we definitely want to use that instead of str. There is also already some snake-to-caml case conversion tooling somewhere (in /tools maybe ?)

@cemerick
Copy link
Contributor Author

cemerick commented Feb 9, 2022

Interesting escape hatch. Why do you need a_unchecked instead of just Unsafe.string_attrib ?

Oh, maybe I don't. I didn't think to (re)use Unsafe bits, but I guess that's what they're there for.

I'll tweak that up and swap Str -> Re.

@cemerick cemerick mentioned this pull request Mar 7, 2022
Closed
@cemerick cemerick force-pushed the master branch 2 times, most recently from 2e8b221 to 2b2e35b Compare March 16, 2022 17:53
@cemerick
Copy link
Contributor Author

I have updated my fork (and this PR) to:

  1. Eliminate the use of Str in favor of Re (the case conversion stuff referenced /tools isn't sufficient; inputs to that kebab_case function can have a mix of kebab, snake, and camel case, alas)
  2. Remove the a_unchecked and `Unchecked additions, using Unsafe.string_attrib instead. Hopefully my handling of the codegen in that case is reasonable (I didn't see any easy way to reuse any of the existing "parsers" there).

Drup
Drup reviewed Mar 17, 2022
View reviewed changes
Copy link
Member

@Drup Drup left a comment

Choose a reason for hiding this comment

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

Nice work !

Could you adjust the documentation/tutorials for the ppx/jsx to mention this new feature ?

jsx/tyxml_jsx.ml Outdated
replace (Posix.compile_pat "[A-Z]") ~f:(fun g -> "-" ^ Group.get g 0) string
|> String.lowercase_ascii
|> replace_string (compile @@ char '_') ~by:"-" in
match exec_opt (Perl.compile_pat {|^(data_?|aria_?)(.+)|}) name with
Copy link
Member

Choose a reason for hiding this comment

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

Please put the compilation part at toplevel !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah I meant to do that before committing 🙃

@cemerick
Copy link
Contributor Author

I've updated to include top-level compilation of the necessary patterns, and added a section to the ppx/jsx docs.

(I haven't been able to "test" the latter though, as I had trouble getting the wikidoc to build; make in the docs directory does produce the API reference, but dies before it generates the narrative wiki content.)

Hopefully I didn't faff up the wiki documentation markup. 🤞

@cemerick cemerick changed the title a_unchecked and _-prefixes in syntax ppx's to support non-standard attribute names Emit Unsafe.string_attrib for _-prefixed attributes in syntax ppxs to support non-standard attribute names Mar 18, 2022
@Drup
Copy link
Member

Drup commented Jun 21, 2022

Thanks @cemerick

@Drup Drup merged commit b291463 into ocsigen:master Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Drup Drup Drup left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide escape hatch for non-standard HTML attributes
2 participants
@cemerick @Drup