Skip to content

Refactoring - #38

Open
rustamwin wants to merge 34 commits into
masterfrom
refactoring
Open

Refactoring#38
rustamwin wants to merge 34 commits into
masterfrom
refactoring

Conversation

@rustamwin

@rustamwinrustamwin commented Sep 22, 2023

Copy link
Copy Markdown
Member
QA
Is bugfix?
New feature?✔️
Breaks BC?✔️/❌
Fixed issues

#22

@rustamwinrustamwin added the status:code review The pull request needs review. label Sep 22, 2023
@rustamwin
rustamwin requested a review from a teamSeptember 22, 2023 05:07
@codecov

codecovBot commented Sep 22, 2023

Copy link
Copy Markdown

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (f8f7776) 98.11% compared to head (5b23a1f) 98.96%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@ Coverage Diff @@## master #38 +/- ##
============================================
+ Coverage 98.11% 98.96% +0.85% - Complexity 20 83 +63 
============================================
Files 1 10 +9 Lines 53 193 +140 ============================================
+ Hits 52 191 +139 - Misses 1 2 +1 
FilesCoverage Δ
src/AbstractClassifier.php100.00% <100.00%> (ø)
src/Filter/ClassAttributes.php100.00% <100.00%> (ø)
src/Filter/ClassImplements.php100.00% <100.00%> (ø)
src/Filter/SubclassOf.php100.00% <100.00%> (ø)
src/Filter/TargetAttribute.php100.00% <100.00%> (ø)
src/NativeClassifier.php100.00% <100.00%> (ø)
src/ReflectionFile.php100.00% <100.00%> (ø)
src/TokenizerClassifier.php100.00% <100.00%> (ø)
src/Filter/Condition/FilterAnd.php88.88% <88.88%> (ø)
src/Filter/Condition/FilterOr.php88.88% <88.88%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@what-the-diff

Copy link
Copy Markdown

PR Summary

  • New Abstract Classifier Structure
    A file named src/AbstractClassifier.php was added. It contains a foundational 'Abstract Classifier' which holds a certain function—find()—among others to aid in locating elements across our system.

  • Classifier Interface Implementation
    src/ClassifierInterface.php was added, this holds the Classifier Interface—a defined set of rules for the 'Classifier' to follow, stipulating that any 'Classifier' needs to have a find() function.

  • Classifier Visitor Addition
    The src/ClassifierVisitor.php file was included. This features a 'Classifier Visitor'—a tool that goes through various 'Classifiers' and retrieves any needed information.

  • PHP Parser Classifier
    The file src/PhpParserClassifier.php has been added, introducing a new 'Classifier' that extends the capacities of the Abstract Classifier. This enables it to parse PHP files and find classes within them, thereby making it easier to understand and manage our codebase.

  • Base Classifier Testing
    The file tests/BaseClassifierTest.php was included, extending our 'Test Case' with specific testing methods for the newly incorporated 'Classifiers' within various scenarios.

  • Revisions to Existing Classifier Test
    The existing tests/ClassifierTest.php file was altered to encompass the functionality of the new 'Base Classifier Test'. This allows it to create a 'PHP Parser Classifier' instance and thus, tests its functionality more efficiently.

Comment threadcomposer.json Outdated
Comment threadsrc/ClassifierInterface.php
Comment threadsrc/PhpParserClassifier.php Outdated
Comment threadsrc/PhpParserClassifier.php Outdated
Comment threadsrc/PhpParserClassifier.php Outdated
Comment threadsrc/Classifier.php Outdated
@rustamwin
rustamwin requested a review from vjikSeptember 28, 2023 06:57
@rustamwinrustamwin changed the title Refactoring & PhpParserClassifierRefactoring & ParserClassifierSep 28, 2023
@xepozz

Copy link
Copy Markdown
Member

Look at this file and get some optimization from the visitor.
Returning false make the parser skip scanning all children nodes.

@vjik

vjik commented Sep 28, 2023

Copy link
Copy Markdown
Member

Look at this file and get some optimization from the visitor. Returning false make the parser skip scanning all children nodes.

Good optimization. But it can be implemented in separate PR.

@rustamwin

rustamwin commented Oct 15, 2023

Copy link
Copy Markdown
MemberAuthor

OK, I removed the parser classifier from the PR (moved it to #40). But I have some things to do here.

@rustamwinrustamwin added status:under development Someone is working on a pull request. and removed status:code review The pull request needs review. labels Oct 15, 2023
@rustamwinrustamwin changed the title Refactoring & ParserClassifierRefactoringOct 15, 2023
@rustamwin

rustamwin commented Oct 27, 2023

Copy link
Copy Markdown
MemberAuthor

Benchmark results:
With XDebug
telegram-cloud-photo-size-2-5321473048660004612-y

Without XDebug
image

{
$classifier = $params['classifier'];
$classifierInstance = new $classifier(...$params['dirs']);
$classifierInstance->find();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add tests for finding:

  • Interfaces
  • Classes implements interfaces
  • Classes inherits other classes

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done.

@vjik

vjik commented Oct 27, 2023

Copy link
Copy Markdown
Member

My benchmark results:

image

image

@rustamwin
rustamwin requested a review from a teamOctober 31, 2023 06:24
@rustamwinrustamwin added status:code review The pull request needs review. and removed status:under development Someone is working on a pull request. labels Oct 31, 2023
Comment threadcomposer.json Outdated
Comment threadcomposer.json Outdated
Comment threadcomposer.json Outdated
Comment threadcomposer-require-checker.json Outdated
Comment thread.github/workflows/bechmark.yml
@rustamwin
rustamwin requested a review from a teamNovember 4, 2023 07:53
*
* @return bool `true` if class matches against filter. Otherwise, `false`.
*/
public function match(ReflectionClass $reflectionClass): bool;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not all classifier implementations use reflections. Better use filter as DTO, and implementation of classifier will filter by itself.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status:code reviewThe pull request needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@rustamwin@xepozz@vjik@StyleCIBot@samdark