Skip to content

Fix #53: improve dependency cycle error message - #61

Merged
greglook merged 9 commits into
amperity:masterfrom
frenchy64:issue-53-improve-cycle-error
Mar 5, 2020
Merged

greglook merged 9 commits into
amperity:masterfrom
frenchy64:issue-53-improve-cycle-error

Conversation

@frenchy64

@frenchy64 frenchy64 commented Feb 7, 2020 •

Copy link
Copy Markdown
Contributor

Closes #53

Error message for cyclic deps now pretty-prints all (unique) cycles in the dependency graph, from smallest to largest. I removed the :input map from the thrown exception, and replaced it with :cycles, the collection of cycles in the same order as printed in the error.

Example deps graph:

{:a #{:b},
 :b #{:c :a},
 :c #{:d},
 :d #{:a :b}}

Example (new) error message:

Execution error (ExceptionInfo) at lein-monolith.dependency/topological-sort (dependency.clj:249).
Dependency cycles detected!

+ :a
^ + :b
|_/

+ :b
^ + :c
|  + :d
|_/

+ :a
^ + :b
|  + :c
|   + :d
|__/

@greglook greglook self-assigned this Feb 8, 2020

@greglook greglook left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for tackling this! The new cycle representations look neat. I have some feedback on style/idioms in the code and a few opportunities to simplify things.

Comment thread src/lein_monolith/dependency.clj Outdated
Comment thread src/lein_monolith/dependency.clj Outdated
Comment thread src/lein_monolith/dependency.clj
Comment thread src/lein_monolith/dependency.clj Outdated
Comment thread src/lein_monolith/dependency.clj Outdated
Comment thread src/lein_monolith/dependency.clj Outdated
Comment thread test/lein_monolith/dependency_test.clj Outdated
Comment thread test/lein_monolith/dependency_test.clj Outdated
Comment thread test/lein_monolith/dependency_test.clj Outdated
Comment thread test/lein_monolith/dependency_test.clj Outdated
@greglook

Copy link
Copy Markdown
Contributor

You should also merge master to pick up some new CI checks.

@frenchy64

Copy link
Copy Markdown
Contributor Author

Merged master, tests pass.

@frenchy64
frenchy64 requested a review from greglook February 17, 2020 15:47
@greglook

Copy link
Copy Markdown
Contributor

Hmm, I wonder why CircleCI isn't running on your commits? 🤔

@greglook
greglook merged commit 4282896 into amperity:master Mar 5, 2020
@drewinglis

Copy link
Copy Markdown
Contributor

Hmm, I wonder why CircleCI isn't running on your commits? 🤔

It's supposed to:

image

Maybe @aengelberg would be willing to look in to this? :^)

@aengelberg aengelberg mentioned this pull request Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve cyclic dependency detection output

3 participants