search

qossmic / deptrac

Keep your architecture clean.
https://qossmic.github.io/deptrac
MIT License
2.6k stars 135 forks source link

Allow deprecated code to be ignored #1359

Closed brightbyte closed 5 months ago

brightbyte commented 7 months ago

When refactoring a framework or library to improve layering, old code often cannot be removed immediately. Instead, it has to be marked as @deprecated, and kept around for a relase or two. When that is the case, we generally want to be able to ignore violations triggered by deprecated code, since we already know that we will remove it as soon as possible, and we want to focus on making the new code clean. Sometimes it's not even possible to avoid the offending dependency in the deprecated code, for structural reasons - that may well have been the reason for deprecating that code in the first place.

The detection logic for deprecated methods and classes is built directly into FilweReferenceVisitor. That's not great, but serves as a proof of concept.

patrickkusebauch commented 7 months ago

I have to say I do not like altering the AstParsing based on a config variable. It feels like lying to me. And I am not even sure how that would work with caching. Is the assumption that we would rebuild the deptrac.cache every time the analyser config is changed?

I propose an alternative solution. Looking at Qossmic\Deptrac\Contract\Dependency\DependencyInterface, we could add one more method getContext, that would provide metadata about under what conditions the dependency occurred. It could provide information about inside what method the dependency occurred for dependencies inside classes. It could provide info about whether the depender class and depender method are deprecated or not.

Users would be then free to write a handler that would then ignore those dependencies.

In general, my philosophy is that deptrac should provide you with the information necessary for you to write custom handlers for the functionality you need. Rather than provide switches that baked the behavior inside deptrac.

What do you think?

brightbyte commented 7 months ago

I have to say I do not like altering the AstParsing based on a config variable. It feels like lying to me. And I am not even sure how that would work with caching. Is the assumption that we would rebuild the deptrac.cache every time the analyser config is changed?

To be honest, for the proof of concept, I didn't think about caching. I just wanted to find a way to make it work, then think about how to make it work more nicely.

I propose an alternative solution. Looking at Qossmic\Deptrac\Contract\Dependency\DependencyInterface, we could add one more method getContext, that would provide metadata about under what conditions the dependency occurred. It could provide information about inside what method the dependency occurred for dependencies inside classes. It could provide info about whether the depender class and depender method are deprecated or not.

That sounds attractive. I'm a bit unsure about how to represent the method, but I'll come up with a proposal.

In general, my philosophy is that deptrac should provide you with the information necessary for you to write custom handlers for the functionality you need. Rather than provide switches that baked the behavior inside deptrac.

Yes, I like that philosophy. I just don't yet know the code well enough to propose structural changes. But I'll give it a go!

patrickkusebauch commented 7 months ago

That sounds attractive. I'm a bit unsure about how to represent the method, but I'll come up with a proposal.

You might want to do something similar to what the FileReferenceVisitor does with currentReference and tokenTemplates. look at enterNode and leaveNode and when you enter/leave a method, call currentReference and set a new property that defines the currentMethod. If you then try to create a new DependencyToken, you can also pass what was the currentMethod where the dependency occurred (if any). You can also pass the method metadata as well.

brightbyte commented 7 months ago

You might want to do something similar to what the FileReferenceVisitor does with currentReference and tokenTemplates. Thanks, I'll have a look!

Re adding a method to DependencyInterface - since it's part of the contract, I am wondering whether extensions are free to implement it? If they are, then adding a method would be a breaking change...

patrickkusebauch commented 7 months ago

The should have no reason to, but you are right. Luckily, we are planning a breaking release soon anyway.

brightbyte commented 7 months ago

The should have no reason to, but you are right. Luckily, we are planning a breaking release soon anyway.

For MediaWiki, we have found it useful to define that implementing interfaces and extending classes is only safe when it explicitly documented to be safe. So a contract interface would be safe to call, but generally not safe to implement.

brightbyte commented 7 months ago

@patrickkusebauch I have pushed a branch that experiments with introducing DependencyContext: https://github.com/brightbyte/deptrac/pull/new/DependencyContext

I'm not really happy with it... Looping the context through the various classes is a bit painful, and setting it to the correct value needs some hackery, because it's hard to pass the requried information into FunctionLikeExtractor. Also, I have been wondering about combining it with FileOccurrance.

I'm curious what you think!

patrickkusebauch commented 7 months ago

I will take a look tomorrow, over the weekend the latest.

patrickkusebauch commented 7 months ago

@brightbyte I took a look. And I think we should not only combine it with FileOccurence, but with DependencyType as well. So at the end you have just Depender, Dependent and Context.

If you'd like, I can write up the refactor into the DependencyContext as a separate PR and you can just piggyback on it with the "state of deprecated" so to speak.

brightbyte commented 7 months ago

@brightbyte I took a look. And I think we should not only combine it with FileOccurence, but with DependencyType as well. So at the end you have just Depender, Dependent and Context.

Yes, that sounds better.

If you'd like, I can write up the refactor into the DependencyContext as a separate PR and you can just piggyback on it with the "state of deprecated" so to speak.

@patrickkusebauch Oh, that would be quite nice, thank you!

By the way... I keep getting confused about "depender" and "dependent". A "depender" is soneone who depends I suppose, but a "dependent" is, according to Webster, also someone who depends... If we are refactoring, would that be a good opportunity to fix the naming?

patrickkusebauch commented 7 months ago

Naming is hard. I was never able to come up with better names.

Other than maybe dependencySource and dependencyTarget I could not imagine better names. We used to have depender and dependee, but that was not much better.

brightbyte commented 6 months ago

If you'd like, I can write up the refactor into the DependencyContext as a separate PR and you can just piggyback on it with the "state of deprecated" so to speak.

@patrickkusebauch I left a couple of comments on your PR, are you intersted in discussing the details of that approach?

I'd like to try implementing "deprecation tagging" on top of your code, but I'm not sure how I can layer my PR on top of yours. AFAIK a PR can't target a branch of a form. Or can it somethow?

patrickkusebauch commented 6 months ago

I will check it tonight.

gennadigennadigennadi commented 5 months ago

Please update the git origin for development to https://github.com/qossmic/deptrac-src and reopen the PR. Thank you very much!