-
-
Notifications
You must be signed in to change notification settings - Fork 217
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
Handle build hangs due to unintended template recursion. #539
Conversation
format the changes Fix suggestions from nightly clippy change error msg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
askama_derive/src/lib.rs
Outdated
let mut check = vec![(input.path.clone(), source)]; | ||
while let Some((path, source)) = check.pop() { | ||
for n in parse(&source, input.syntax)? { | ||
match n { | ||
Node::Extends(Expr::StrLit(extends)) => { | ||
let extends = input.config.find_template(extends, Some(&path))?; | ||
let dependency_path = format!("{:?} --> {:?}", path, extends); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just store (path, &extends)
in the dependency_graph
, and only format when yielding an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review :)
dependency_graph
is a vector
of String
, that can be easily converted to a tuple
of (PathBuf, PathBuf)
but what my concern is that lifetime
of dependency_graph
, extends
and path
is diff so there will be compiler issues that extends/path does not live long enough
when I will push
values in the vector
.
Please, let me know if I am missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you referring to get the path
and extends
to String
and push
the owned String
into the dependency_graph
vector
?
In my opinion, it will not add much since we need the step to convert it into String
format anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two concerns here:
- Should avoid unnecessary allocations outside the error path
- Would prefer to store structured data than "humanized" data
(PathBuf, PathBuf)
seems like a fine option. It does look like both path
and extends
might be owned here anyway, such that we don't have to do extra allocations to store them in dependency_graph
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done some changes pls take a look.
Thanks for fixing this! |
Created a vector to keep track of source and extends dependency.
If the same dependency comes again throw the Err as there is some cyclic dependency between source and extends file.
Case 1: When there is a self-loop in dependency
Case 2: When there is a cyclic dependency