search

commonsensesoftware / more-rs-di

Rust Dependency Injection (DI) framework
MIT License
20 stars 3 forks source link

Feature/use graphs to detect circular references #9

Closed jescobar closed 1 year ago

jescobar commented 1 year ago
jescobar commented 1 year ago

Hi @commonsensesoftware, please excuse me for late reply, I was able to add a couple of complex unittests that were failing with the previous implementation, also I have modified the internal logic to use nodes and edges in an acyclic graph in order to detect these complex scenarios, I hope this help, please let me know what you think about this new implementation and/or if it is fine, Thanks!

commonsensesoftware commented 1 year ago

@jescobar thank you so much for taking the time to produce the scenario. The test case and setup is what I was the most interested in. There may be more.

Although I know a graph can be used, I'm not convinced it's really necessary. In particular, I really want to avoiding addition crate dependencies just for validation unless it's absolutely necessary. The structure and evaluation is still ultimately a graph, even if it isn't represented that way as first-class data structure.

I reverted back to the HashMap I was using to track visited types, but I fixed the logic so that it now does something (as I discovered it was doing nothing). I've created PR #10 so that you can compare and contrast my revision to this PR. I forked over your setup and test cases. I ran your test cases prior to any changes and confirmed there was definitely a bug. With a very small amount of refactoring, all of the old and new test cases pass without having to use a graph.

Let me know what you think when you have a moment. If there are other edge cases that I may have missed, we should add those too.

jescobar commented 1 year ago

@commonsensesoftware, you're welcome, it is great that you figure out a way to pass the tests withouth the explicit graph! Regarding the tests, I don't remember any other scenario or corner case, so I think (but I can be wrong) those tests are the ones coverying all the possible circular dependency scenarios.

so I think I can close (dismiss) this PR in favor of your new PR!

commonsensesoftware commented 1 year ago

Sweet! Thank you. I wanted to bounce that off of you before proceeding. You definitely found an issue so I'm glad we resolved that. I'll try to have the changes published by tomorrow.

commonsensesoftware commented 1 year ago

FYI, 2.1.1 was published to crates.io with the changes. Thanks again.

jescobar commented 1 year ago

@commonsensesoftware thanks a lot! I will use the new version today!