Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor DependencyInterface #1368

Closed
wants to merge 2 commits into from
Closed

Refactor DependencyInterface #1368

wants to merge 2 commits into from

Conversation

patrickkusebauch
Copy link
Collaborator

to use DependencyContext instead of FileOccurence and DependencyType, so that we do not add a new method to the interface and constructor parameters every time we want to add a piece of information about the dependency.

Created to enable #1359

BC break: Yes

…Occurence and DependencyType, so that we do not add a new method to the interface and constructor parameters every time we want to add a piece of information about the dependency.

Created to enable #1359
…Occurence and DependencyType, so that we do not add a new method to the interface and constructor parameters every time we want to add a piece of information about the dependency.

Created to enable #1359
@patrickkusebauch patrickkusebauch self-assigned this Feb 7, 2024
@@ -50,8 +55,7 @@ public function variable(string $classLikeName, int $occursAtLine): self
{

Choose a reason for hiding this comment

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

It's not clear to me how a caller could provide additional information about the dependency this way. It seems like we'd have to replace the $occursAtLine parameter with $context in all methods here, so the caller can control the information in the context. That way, we can add more information to the context without having to change all the methods in ReferenceBuilder again.

However, that means that the dependency type should probably not be part of the context.

Choose a reason for hiding this comment

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

In any case, thank you for doing this, having DependencyContext available will make it a lot easier to implement the features we discussed.

Choose a reason for hiding this comment

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

Hm, I guess we could also implement the "deprecated context" by creating a new ReferenceBuilder when entering a deprecated scope. But I'd still prefer to replace $occursAtLine - the line currrent is the context, we are just providing a wrapper that makes it future-proof.

Copy link
Collaborator Author

@patrickkusebauch patrickkusebauch Mar 1, 2024

Choose a reason for hiding this comment

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

This is contextual information, not information that you find out when the dependency is created. So I would imagine something like this:

  • When you enterNode a deprecated node in FileReferenceVisitor, you call $this->currentReference->setIsInDeprecetedContext(true) or something similar. This stores an instance property inside a ReferenceBuilder.
  • When you leaveNode, you set it to false in the same manner.
  • Update ReferenceBuilder::createContext to pass the instance property to the DependencyContext value object constructor
  • Propagate it forward where appropriate
  • Use the information in the event processing to stop raising violations.

Does that make sense?

@gennadigennadigennadi
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants