Adding Fody Janitor attribute to communicate intend and assure presence of Janitor
Closed this issue · 8 comments
We started to make use of Janitor in one of our projects and ran in the situation in which it was not clear why an object implemented an empty Dispose method. This can of course be clarified by adding a comment. However it got me thinking, wouldn't it be better to add an attribute which could (optionally) be placed on Dispose methods. This would serve two purposes, communicate intend, lead to build failure if Janitor is accidentally removed.
The attribute could be named JanitorManaged for example.
class Foo : Disposable
{
[JanitorManaged]
public void Dispose()
{}
}Please share your thoughts on this.
would not a comment suffice?
It would suffice to communicate intend, however that leaves the second issue that Janitor could be accidentally removed without causing a build failure. Additionally adding having an Attribute could add a bit of consistency over different comments.
the second issue that Janitor could be accidentally removed without causing a build failure
i recommend adding a unit test around your disposable objects to ensure they act as expected when they are disposed. this way if someone accidentally removes Janitor u will get a failed test
True. However I don't see a downside of adding such a Attribute. Any change of a pull request containing this getting merged?
FWIW, the SkipWeaving attribute already exists, so if you are looking for a way to have your build fail if you accidentally remove Janitor, you could do something like this:
#if DEBUG
[SkipWeaving]
public class EnsureJanitor : IDisposable
{
public void Dispose() {}
}
#endifwouldnt this be better asserted with a unit test?
Oh, yes, I totally agree with you on that. I'm just saying that IF you're looking for a way to do this with a build failure instead of with a test failure, this may be a way to go.
sorry i dont think this proposal adds any value