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.

Sign up for GitHub

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

Jump to bottom

Fix 758 #778

Merged
merged 4 commits into from
Jan 10, 2022
Merged

Fix 758 #778

merged 4 commits into from
Jan 10, 2022

Conversation

mdevlamynck
Copy link
Contributor

@mdevlamynck mdevlamynck commented Jan 1, 2022 •
edited

Fixes #758.

Prevent namespaces from being added to use dependencies. Namespaces are
detected by looking if the used FQDN is a prefix of some class FQDN.

This strategy may be a bit brittle, I'll let you judge if this good enough or if the current behavior is better.

@mdevlamynck mdevlamynck force-pushed the fix-758 branch 2 times, most recently from 105f687 to 4daf071 Compare January 1, 2022 20:00
dbrumann
dbrumann reviewed Jan 6, 2022
View reviewed changes
Copy link
Collaborator

@dbrumann dbrumann left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good to me. Do you think you can write a test case for this?

src/DependencyEmitter/UsesDependencyEmitter.php Outdated Show resolved Hide resolved
patrickkusebauch
patrickkusebauch suggested changes Jan 6, 2022
View reviewed changes
src/DependencyEmitter/UsesDependencyEmitter.php Outdated
{
$dependencyFQDN = $dependency->getTokenName()->toString();

foreach ($astMap->getAstClassReferences() as $class) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You might want to do a same type of for each loop for functions here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, nice catch :)

@mdevlamynck
Copy link
Contributor Author

Yes I'll add tests :) Specifically the only issue I can foresee with this approach is considering that Qossmic\Deptrac\AstRunner\AstMap is a namespace (because it's a prefix of Qossmic\Deptrac\AstRunner\AstMap\AstDependency for instance) while it's also a FQDN. I'll definitely try to test that.

@mdevlamynck mdevlamynck force-pushed the fix-758 branch 4 times, most recently from 73ed043 to b8e34ee Compare January 6, 2022 09:25
mdevlamynck
mdevlamynck commented Jan 6, 2022
View reviewed changes
src/DependencyEmitter/UsesDependencyEmitter.php Outdated
{
$dependencyFQDN = $dependency->getTokenName()->toString();

$functionReferences = array_merge(...array_values(array_map(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a better way to get all function references from AstMap? There is a list of function references in AstMap but no getter exposes it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you need at and there is no nice way to get it, just add the getter. :)

@mdevlamynck
Fix 758
d23e69c 
Prevent namespaces from being added to use dependencies. Namespace are
detected by looking if the used FQDN is a prefix of some class FQDN.
@mdevlamynck mdevlamynck force-pushed the fix-758 branch 2 times, most recently from b543e16 to 87c3bfb Compare January 8, 2022 10:41
@mdevlamynck
Copy link
Contributor Author

Tests added :)

dbrumann
dbrumann approved these changes Jan 10, 2022
View reviewed changes
Copy link
Collaborator

@dbrumann dbrumann left a comment

Choose a reason for hiding this comment

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

Thank you again. Great work 👍

@dbrumann dbrumann merged commit cb2bf57 into qossmic:main Jan 10, 2022
@pincher2012
Copy link

pincher2012 commented Jan 20, 2022 •
edited

Hello, thank You for this tool and your contribution!

I just want to share performance results on my project before and after this PR.

Before:

 -------------------- -------- 
  Report                       
 -------------------- -------- 
  Violations           0       
  Skipped violations   901     
  Uncovered            11857   
  Allowed              106775  
  Warnings             0       
  Errors               0       
 -------------------- -------- 

real    0m8,483s
user    0m7,044s
sys     0m1,216s

After:

 -------------------- -------- 
  Report                       
 -------------------- -------- 
  Violations           0       
  Skipped violations   901     
  Uncovered            11464   
  Allowed              106775  
  Warnings             0       
  Errors               0       
 -------------------- -------- 


real    3m51,235s
user    3m49,009s
sys     0m1,367s

How do you think, is it possible to optimize the algorythm?

@pincher2012 pincher2012 mentioned this pull request Jan 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrickkusebauch patrickkusebauch patrickkusebauch requested changes

@dbrumann dbrumann dbrumann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Imports with a parent namespace are marked uncovered
4 participants
@mdevlamynck @pincher2012 @patrickkusebauch @dbrumann