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

Add non-ignorable error handling feature #66

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

Spomky
Copy link
Contributor

@Spomky Spomky commented Aug 12, 2024

Proposal for #65.
Instead of returning a list of strings, the extension now returns a RuleError object built using the RuleErrorBuilder.

Note that I am not familiar with PHPStan extension development and this PR is a very first attempt to solve the issue I created.

Please note that this PR also sets identifiers to the errors. They are namely:

  • bannedcode.function
  • bannedcode.expression
  • bannedcode.test

Let me know if these IDs are suitable or if you prefer to define others.

@Spomky Spomky force-pushed the features/error-non-exclusion branch 2 times, most recently from e8bc44c to 580f2f5 Compare August 12, 2024 15:52
Copy link
Member

@mremi mremi left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution, could you please update CHANGELOG.md as well?

src/Rules/BannedNodesErrorBuilder.php Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Rules/BannedNodesRule.php Outdated Show resolved Hide resolved
src/Rules/BannedUseTestRule.php Outdated Show resolved Hide resolved
src/Rules/BannedNodesErrorBuilder.php Outdated Show resolved Hide resolved
src/Rules/BannedNodesErrorBuilder.php Outdated Show resolved Hide resolved
@Spomky Spomky force-pushed the features/error-non-exclusion branch from 580f2f5 to 9a490f2 Compare August 13, 2024 14:12
@Spomky
Copy link
Contributor Author

Spomky commented Aug 13, 2024

Thanks for this contribution, could you please update CHANGELOG.md as well?

Yes, sure.

What is the next version? Should be v2.1.0 right?

@Spomky Spomky force-pushed the features/error-non-exclusion branch from 9a490f2 to b66491b Compare August 13, 2024 14:17
@mremi
Copy link
Member

mremi commented Aug 13, 2024

Thanks for this contribution, could you please update CHANGELOG.md as well?

Yes, sure.

What is the next version? Should be v2.1.0 right?

yes perfect

@mremi
Copy link
Member

mremi commented Aug 13, 2024

@Spomky the CI is failing, could you please check it?

@Spomky Spomky force-pushed the features/error-non-exclusion branch from b66491b to 0641be9 Compare August 13, 2024 15:43
@Spomky
Copy link
Contributor Author

Spomky commented Aug 13, 2024

Looks good on my side. I missed to verify the error implements all expected interfaces

Extend the `BannedNodesRule` and `BannedUseTestRule` classes to support non-ignorable error handling. Introduced the `BannedNodesErrorBuilder` class to streamline error message construction and updated the configuration and tests accordingly.
@Spomky Spomky force-pushed the features/error-non-exclusion branch from 0641be9 to c3d6975 Compare August 13, 2024 16:01
@mremi mremi merged commit e1c3e47 into ekino:master Aug 14, 2024
3 checks passed
@mremi mremi mentioned this pull request Aug 14, 2024
@mremi
Copy link
Member

mremi commented Aug 14, 2024

New release v2.1.0 is available, thanks a lot @Spomky

@Spomky Spomky deleted the features/error-non-exclusion branch August 14, 2024 09:23
@Spomky
Copy link
Contributor Author

Spomky commented Aug 14, 2024

🎉. I am happy to contribute.

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