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

Use Askama in cpp2 #338

Merged
merged 14 commits into from
Jul 11, 2023
Merged

Use Askama in cpp2 #338

merged 14 commits into from
Jul 11, 2023

Conversation

sffc
Copy link
Contributor

@sffc sffc commented Jun 13, 2023

If you like the general direction this is taking, I'll finish it up for Cpp2.

@sffc sffc requested a review from Manishearth June 13, 2023 02:16
@Manishearth Manishearth changed the title Experiment with Askmara in cpp2 Experiment with Askama in cpp2 Jun 13, 2023
Copy link
Contributor

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

this seems promising

might want to consider moving template def code outside of ty.rs . Unsure if it will be better, worth considering

@@ -0,0 +1,35 @@
#ifndef {{ header_guard }}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: probably should use .hpp or .hpp.tmpl

(and set gitattributes to highlight tmpl as Jinja or something)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll use .h.jinja since that extension is picked up by the plugins for VSCode and Sublime Text

https://marketplace.visualstudio.com/items?itemName=samuelcolvin.jinjahtml

#define {{ header_guard }}

{%~ match decl_include %}
{% when Some with (include) ~%}
Copy link
Contributor

Choose a reason for hiding this comment

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

not quite happy with needing a full match here but i guess it's fine. Does seem to be the idiomatic way to do things (djc/askama#257)

@sffc sffc changed the title Experiment with Askama in cpp2 Use Askama in cpp2 Jul 9, 2023
@sffc sffc marked this pull request as ready for review July 9, 2023 10:21
@sffc
Copy link
Contributor Author

sffc commented Jul 9, 2023

I finished migrating everything that made sense to migrate in Cpp2.

@sffc
Copy link
Contributor Author

sffc commented Jul 9, 2023

The only commit that affects the output is f935c12. I tried to keep the Askama output identical to the pre-existing outupt.

@@ -0,0 +1,29 @@
inline {##}
{{- m.return_ty }} {##}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: when the open or close tag has a - it means to strip whitespace before or after it. The {##} is a comment that protects whitespace.

@sffc sffc requested a review from Manishearth July 9, 2023 10:26
Copy link
Contributor

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

this looks great!

return_ty: Cow<'a, str>,
type_name: Cow<'a, str>,
method_name: Cow<'a, str>,
pre_qualifiers: Vec<Cow<'a, str>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: docs for the non-obvious ones, please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tool/src/cpp2/ty.rs Outdated Show resolved Hide resolved
@sffc sffc requested a review from Manishearth July 11, 2023 07:33
@sffc sffc merged commit ffeb2bd into rust-diplomat:main Jul 11, 2023
@sffc sffc deleted the askmara-exp branch July 11, 2023 08:12
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