Skip to content
This repository was archived by the owner on Mar 11, 2025. It is now read-only.

Add deref builtin filter #1021

Merged
merged 3 commits into from
May 2, 2024
Merged

Add deref builtin filter #1021

merged 3 commits into from
May 2, 2024

Conversation

GuillaumeGomez
Copy link
Collaborator

Since we have as_ref, sometimes it's better to remove references than to add more. :)

Copy link
Member

@Kijewski Kijewski left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@GuillaumeGomez GuillaumeGomez enabled auto-merge (rebase) April 28, 2024 10:50
@GuillaumeGomez GuillaumeGomez disabled auto-merge May 2, 2024 15:40
@GuillaumeGomez GuillaumeGomez enabled auto-merge (rebase) May 2, 2024 15:40
@GuillaumeGomez
Copy link
Collaborator Author

GuillaumeGomez commented May 2, 2024

I think something is missing here? Nothing is happening. ^^'

cc @djc

@Kijewski Kijewski disabled auto-merge May 2, 2024 15:55
@Kijewski
Copy link
Member

Kijewski commented May 2, 2024

Huh, yeah, I too can only activate automerge, but can't manually merge the PR. I didn't even know that automerge was a thing. :D

@Kijewski Kijewski self-requested a review May 2, 2024 18:33
@Kijewski Kijewski merged commit 0cb9fbb into askama-rs:main May 2, 2024
@Kijewski
Copy link
Member

Kijewski commented May 2, 2024

Argh, find the error:

(green circle might be related)

@GuillaumeGomez GuillaumeGomez deleted the deref-builtin branch May 2, 2024 19:08
@GuillaumeGomez
Copy link
Collaborator Author

Dark magic. Good catch! :)

Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

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

I had an unsubmitted comment sitting here, maybe that has something to do with the PR getting stuck on auto-merge?

@@ -331,6 +331,12 @@ fn test_flat_deep() {
#[template(path = "let-base.html")]
struct LetBase {}

#[test]
fn test_let_base() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just suppress the lints instead? Or is this adding useful test coverage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The lint is valid, but allowing it for this struct would have been better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Want to put up a PR that removes the tests in favor of suppressing the lints here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed it in the last commit of #1029.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah we commented at the same time haha. So like I said, last commit of #1029. Can send it as a standalone PR if you prefer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, folding it into the other PR is fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants