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

derive: allow supplying source in comment #127

Merged
merged 12 commits into from
Aug 20, 2024

Conversation

Kijewski
Copy link
Collaborator

Using #[template(source = "…")] is tiresome, because you have to escape the input. That makes it difficult to read, too. Using #[template(path = "…") can be tiresome, too, if the code is small and/or if you just want to prototype something.

This PR adds a third option to supply the source of a template: You can supply the source in the comments of a template. This works similar to doctest. Any rinja block gets extracted, and the combined blocks are the source code of the template.

@GuillaumeGomez
Copy link
Contributor

I like the idea! Don't forget to test with /** and #[doc = ""] too. 😉

@Kijewski Kijewski force-pushed the pr-code branch 3 times, most recently from c19e0fa to e801a93 Compare August 12, 2024 17:01
@Kijewski
Copy link
Collaborator Author

I like the idea! Don't forget to test with /** and #[doc = ""] too. 😉

Oof, yeah, my code did not work at all with these two. :) Fixed.

@GuillaumeGomez
Copy link
Contributor

Does it work with code examples like:

///     let x = 12;
///
/// `````
/// let x = 12;
/// `````

?

If not, I'd recommend using pulldown-cmark to parse the markdown and extract code blocks from it instead of trying to do it yourself. ;)

@Kijewski
Copy link
Collaborator Author

Does it work with code examples like: `````

Oh, I didn't think about this case. Implemented.

@GuillaumeGomez
Copy link
Contributor

It's just one of many. Not sure if your code would handle:

/// `````
/// ```rinja
/// {{bla}}
/// ```
/// `````

correctly

@Kijewski
Copy link
Collaborator Author

Kijewski commented Aug 12, 2024

It's just one of many. Not sure if your code would handle:

It does. I had to turn one panic into an error so I could add the test.

@Kijewski
Copy link
Collaborator Author

I use pulldown-cmark, now. It has a different opinion how to interpret ↓, but I guess it knows it better. :)

/// ```rinja
/// `````
/// {{bla}}
/// `````
/// ```

@Kijewski
Copy link
Collaborator Author

Should we wait until pulldown-cmark/pulldown-cmark#916 is released to keep our MSRV?

@GuillaumeGomez
Copy link
Contributor

Oh that'd be convenient. Another thing I thought about: should we make it an optional feature?

@Kijewski
Copy link
Collaborator Author

Kijewski commented Aug 13, 2024

Another thing I thought about: should we make it an optional feature?

Yes, good call! It will probably stay a very niche feature. What would you call the feature flag? Is "code-in-doc" too long?

@GuillaumeGomez
Copy link
Contributor

code-in-doc sounds good to me. 👍

@Kijewski Kijewski changed the title derive: allow supplying source in comment derive: allow supplying source in comment (waiting for next pulldown-cmark release) Aug 13, 2024
@Kijewski Kijewski force-pushed the pr-code branch 2 times, most recently from 695bd53 to 3f43e43 Compare August 13, 2024 16:11
@Kijewski Kijewski force-pushed the pr-code branch 2 times, most recently from 61ae0ee to d984cd5 Compare August 13, 2024 18:26
fn collect_comment_blocks(ast: &syn::DeriveInput) -> Option<(Option<Span>, String)> {
let mut span: Option<Span> = None;
let mut assign_span = |kv: &syn::MetaNameValue| {
// FIXME: uncomment once <https://github.com/rust-lang/rust/issues/54725> is stable

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment
@GuillaumeGomez
Copy link
Contributor

Oh btw I asked on the pulldown-cmark PR and they said hopefully there should be a release this week. :)

@GuillaumeGomez
Copy link
Contributor

New pulldown-cmark version with reduced MSRV has been published.

@Kijewski Kijewski changed the title derive: allow supplying source in comment (waiting for next pulldown-cmark release) derive: allow supplying source in comment Aug 16, 2024
@Kijewski
Copy link
Collaborator Author

Kijewski commented Aug 16, 2024

error: package pulldown-cmark v0.11.1 cannot be built because it requires rustc 1.71.1 or newer, while the currently active rustc version is 1.71.0

I guess it's acceptable if I raise our MSRV by 0.1. :D

Well, let's not. If the user does not need the feature, then 1.17.0 is good enough.

@Kijewski Kijewski marked this pull request as ready for review August 16, 2024 16:49
@Kijewski
Copy link
Collaborator Author

Hm, should #[template(in_doc = false)] succeed if the feature is not implemented? I guess yes?

@Kijewski Kijewski force-pushed the pr-code branch 2 times, most recently from f41c0b0 to 90d5a79 Compare August 20, 2024 10:07
@GuillaumeGomez
Copy link
Contributor

I'd say yes.

As an alternative to supplying the code template code in an external file (as `path` argument),
or as a string (as `source` argument), you can also enable the `"code-in-doc"` feature.
With this feature, you can specify the template code directly in the documentation
of the template `struct`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
of the template `struct`.
of the template item.

Because I think we support union or enum (or both). Never remember which.

@Kijewski
Copy link
Collaborator Author

I'd say yes.

I already, pro-actively made this case succeed. :)

of the template `struct`.

Instead of `path = "…"` or `source = "…"`, specify `in_doc = true` in the `#[template]` attribute,
and in the struct's documentation add a ```` ```rinja ```` code block:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
and in the struct's documentation add a ```` ```rinja ```` code block:
and in the item's documentation, add a code block with the `rinja` attribute:

@@ -1,6 +1,6 @@
[package]
name = "rinja_actix"
version = "0.3.0"
version = "0.3.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

A reason why you're updating versions? Shouldn't it be done in a separate PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because I added a new feature, but it could be done in another PR, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not the first one. Please do it in another PR then. Like that we make publish it too and also ensure we don't have any other features we want to be merged before making this release.

CompileError::new("template `path` or `source` not found in attributes", None)
CompileError::new(
#[cfg(not(feature = "code-in-doc"))]
"specify one template argument `path` OR `source`",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe let's not put OR in capital? XD

@@ -422,6 +433,177 @@ impl TemplateArgs {
}
}

/// Try to find the souce in the comment, in a "```rinja```" block
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Try to find the souce in the comment, in a "```rinja```" block
/// Try to find the souce in the comment, in a `rinja` code block.

let kind = match &ast.data {
syn::Data::Struct(_) => "struct",
syn::Data::Enum(_) => "enum",
syn::Data::Union(_) => "union",
Copy link
Contributor

Choose a reason for hiding this comment

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

So we do support all item types! 😆

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hehe, I simply added cases for all variant of the enum. I actually don't think that we generate valid code for unions, because I think reading from unions is only allowed in an unsafe context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Funny. Might worth handling unions correctly at some point.

/// of the template `struct`.
///
/// Instead of `path = "…"` or `source = "…"`, specify `in_doc = true` in the `#[template]`
/// attribute, and in the struct's documentation add a ```` ```rinja ```` code block:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// attribute, and in the struct's documentation add a ```` ```rinja ```` code block:
/// attribute, and in the struct's documentation add a `rinja` code block:

@Kijewski
Copy link
Collaborator Author

All suggestions applied.

@GuillaumeGomez
Copy link
Contributor

Thanks a lot, it's a very cool feature! :)

@GuillaumeGomez GuillaumeGomez merged commit 4ef3567 into rinja-rs:master Aug 20, 2024
29 checks passed
@Kijewski
Copy link
Collaborator Author

Thanks! And thank you for your reviews and suggestions! :)

@Kijewski Kijewski deleted the pr-code branch August 20, 2024 12:59
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