Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

tsort command might not be available on all OS. #6

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

sziep commented Jun 28, 2013

After a bit more testing on windows, a couple of issues popped up. The tsort command was unknown, and the path definition for dep-concat from the Gruntfile have to be converted as well. Anyways, this is now working on windows for real :)

S: would be cool if, after you accept the pull request, you could update the npm module as well.

Thanks,
-Stephan

Owner

tJener commented Jun 28, 2013

@sziep On a slight tangent, I unfortunately don't have a good way to testing this on Windows, would it be ok if I add you as a contributor?

Back to the PR—cycles in the graph cause grunt to die without a message. I'll push a test case up sometime later today or the weekend, or if you get around to adding on to this PR, that'd also work.

Unix's tsort, when it detects a cycle, also prints out the detected cycle as a part of the failure. However, the tsort library you are using does not do that. Though not completely necessary, this is rather useful once the project gets large (100+ files [although you could argue that at that point, you should be using a more robust module loader like AMD or CJS]).

What are your thoughts? I'm only really hesitant because it removes the existing feature of listing out the detected cycle.

Also worth changing would be this section of the README. It currently states the Unix tsort requirement, which this patch would remove.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment