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

code refactor #364

Merged
merged 17 commits into from
Jun 7, 2023
Merged

code refactor #364

merged 17 commits into from
Jun 7, 2023

Conversation

xHeaven
Copy link
Contributor

@xHeaven xHeaven commented May 17, 2023

This is a code refactor. Read the commit messages and descriptions for more information on each commit.

xHeaven added 13 commits May 18, 2023 00:17
The PHPDoc return type hint was incomplete, it should have been `InvalidEmail|null`, however, it can be removed altogether as the return type is already inferred from the code.
This is not a breaking change as the method is private,  and it's only used at one place, where the return value was not used anyway.
`private $parser` was used in only one place, hence it can be local, there is no reason to put it into the class' scope.
Concatenation already casts `static::CODE` from `int` to `string`, no reason to do it explicitly.
- fixed numbering at the `Available validations` section
- fixed overall formatting
@egulias
Copy link
Owner

egulias commented May 23, 2023

Hi @xHeaven , thanks for the PR.
It does have a lot of changes, I'll be reviewing it.
Meanwhile, please review Psalm errors (you should be able to run it locally too)

ERROR: InvalidPropertyAssignmentValue - src/EmailLexer.php:161:28 - $this->nullToken 
with declared type 'Doctrine\Common\Lexer\Token<int, string>' 
cannot be assigned type 'Doctrine\Common\Lexer\Token<, "">' (see https://psalm.dev/145)
        $this->nullToken = $nullToken;
ERROR: InvalidPropertyAssignmentValue - src/EmailLexer.php:163:26 - $this->current 
with declared type 'Doctrine\Common\Lexer\Token<int, string>' 
cannot be assigned type 'Doctrine\Common\Lexer\Token<, "">' (see https://psalm.dev/145)
        $this->current = $this->previous = $this->nullToken;
ERROR: InvalidPropertyAssignmentValue - src/EmailLexer.php:163:44 - $this->previous 
with declared type 'Doctrine\Common\Lexer\Token<int, string>' 
cannot be assigned type 'Doctrine\Common\Lexer\Token<, "">' (see https://psalm.dev/145)
        $this->current = $this->previous = $this->nullToken;

Thanks!

@xHeaven
Copy link
Contributor Author

xHeaven commented May 24, 2023

Hey @egulias, the Psalm errors should be fixed.
Apparently, even though the type hint that I've just reverted is absolutely redundant in the EmailLexer, it's needed, because Psalm can't seem to do its static analysis without it.
I suppose that was the original reason for having a dummy variable called $nullToken as well.
Too bad, weird stuff I guess.

@xHeaven
Copy link
Contributor Author

xHeaven commented May 25, 2023

Is there anything I can do to help successfully execute the currently failing test? I've got 9-ish emails about it so far, let me know if there's anything I can do. @egulias

@egulias
Copy link
Owner

egulias commented May 26, 2023

Hey @xHeaven , apologies for the spam! I didn't realise you were getting the emails too.
The issue is that for some reason I haven't found yet, GitHub's action is not bringing the value of the secret ´CODACY_PROJECT_TOKEN´ (here), that it has worked few days ago when I first introduced it.

Will use this PR to test, you might get more spam, apologies in advance now :).

Still working on the review, thanks for your patience!

@xHeaven
Copy link
Contributor Author

xHeaven commented May 26, 2023

No worries, keep me updated! :)

Copy link
Owner

@egulias egulias left a comment

Choose a reason for hiding this comment

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

Hey. Thanks for waiting.
I have left some comments for you. Many are aesthetics more than anything, that I'd like to keep.
A couple need some conversation.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Parser/CommentStrategy/DomainComment.php Show resolved Hide resolved
src/Validation/DNSRecords.php Outdated Show resolved Hide resolved
src/Validation/DNSRecords.php Outdated Show resolved Hide resolved
src/Validation/MultipleValidationWithAnd.php Outdated Show resolved Hide resolved
src/Parser/Comment.php Show resolved Hide resolved
Also using constructor property promotion, see more info about it [here](https://php.watch/versions/8.0/constructor-property-promotion).
@xHeaven
Copy link
Contributor Author

xHeaven commented Jun 1, 2023

Hey @egulias,

I'm done with the requested changes, would you take a look at it whenever you can?
Also, I've used constructor property promotion in my last commit, is that fine, or would you like to stick with the old-style property definition?

@xHeaven xHeaven requested a review from egulias June 1, 2023 12:50
Copy link
Owner

@egulias egulias left a comment

Choose a reason for hiding this comment

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

Looks good! Will take the chance to see if I can manually send the coverage results and then will merge.
Thanks for the effort!

@codacy-production
Copy link

Coverage summary from Codacy

Merging #364 (5ca02b4) into 4.x (97c28cd) - See PR on Codacy

Coverage variation Diff coverage
+0.34% (target: -1.00%) 87.50%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (97c28cd) 691 635 91.90%
Head commit (5ca02b4) 708 (+17) 653 (+18) 92.23% (+0.34%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#364) 24 21 87.50%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

@egulias egulias merged commit 27be0e7 into egulias:4.x Jun 7, 2023
@egulias
Copy link
Owner

egulias commented Jun 7, 2023

Thanks again!

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.

2 participants