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

remove an edge when there's cycles #2

Merged

Conversation

matthewmueller
Copy link
Contributor

Okay I wanted to offer a solution. I haven't thought about all the possibilities here, but I think this would work.

Basically grab all the cycles, and remove the edge that completes the cycle, before doing the topological sort.

I've tested this on an app with cyclical dependencies and it seems to work fine. Unfortunately there are other parts of mako that throw with cycles, so they'll need to be updated too.

@dominicbarnes
Copy link
Owner

This function should not be manipulating the graph, we need to find a better solution. I checked out the implementation in graph.js, but they throw errors for cycles too.

We'll just need to add some tests and find a solution for this, I can work on it tonight.

@dominicbarnes
Copy link
Owner

Oh hahahah, that's a clone of the graph we are working on, so manipulating it is fine. Sorry about that @matthewmueller!

@dominicbarnes
Copy link
Owner

This solves #1

dominicbarnes added a commit that referenced this pull request Feb 8, 2016
remove an edge when there's cycles
@dominicbarnes dominicbarnes merged commit 3c2e4a6 into dominicbarnes:master Feb 8, 2016
@matthewmueller
Copy link
Contributor Author

sweet, glad we're on the same page that this (unfortunately) needs to get sorted out.

i think the only other piece is updating mako-tree to internally handle it for method calls like .removeFile, which throw if there are cycles. I'll get something working with the app I'm testing it on and then open a PR with some changes.

@matthewmueller matthewmueller deleted the sort-with-cycles branch February 9, 2016 03:56
@dominicbarnes
Copy link
Owner

It looks like the changes made in this PR have negative consequences after all. I'm working on adding a cycle test in mako-js, and with a case like this: (borrowed from here)

index: a, b
a: b
b: a

This lib returns an empty array for the toposort, which means mako stops doing processing altogether. It looks like I'm going to need to find a proper toposort algorithm to port for handling cycles.

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.

None yet

2 participants