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 the ability to optionally merge automatically inferred rules with manual rules #848

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

CWAscend
Copy link

@CWAscend CWAscend commented Aug 19, 2024

💪 Motivation

I would like the ability to use a combination of Attribute rules and manual rules that are defined within the static rules method in a Data object. For me, this will massively clean up my rules method when I have to reach for it, such as which I might be reaching for a callable rule, or using the Rule::in() rule where the array is dynamic - just to name some examples.

🛠 Changes

  • Added a new Spatie\LaravelData\Attributes\MergeRules attribute
  • This will merge the rules from the rules method rather than overwriting them
  • Example usage can be found in the test Data class here: tests/Fakes/DataWithMergedRuleset.php

🧪 Testing

  • A new test has been created: it will merge validation rules, which ensures both validation rules from the Attributes and the rules method are picked up.
  • A new test has been created: it will merge validation rules using string rules which ensures the validation rules can be a piped string of rules instead of an array of files for each attribute

🧙 Reminders (Author)

  • Have you read through your own diff? 👀
  • Are all tests passing? 🧪

@CWAscend CWAscend changed the title Add the ability to optionally merge attribute rules with method rules Add the ability to optionally merge automatically inferred rules with manual rules Aug 19, 2024
@CheesyTech
Copy link

Up

Copy link
Member

@rubenvanassche rubenvanassche left a comment

Choose a reason for hiding this comment

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

Good idea! Some changes required in order to get it merged.

src/Resolvers/DataValidationRulesResolver.php Outdated Show resolved Hide resolved
src/Resolvers/DataValidationRulesResolver.php Outdated Show resolved Hide resolved
src/Resolvers/DataValidationRulesResolver.php Outdated Show resolved Hide resolved
src/Resolvers/DataValidationRulesResolver.php Outdated Show resolved Hide resolved
tests/Fakes/DataWithMergedRuleset.php Outdated Show resolved Hide resolved
@CWAscend
Copy link
Author

@rubenvanassche thanks for the feedback, I'll action this over the weekend 👍

@CWAscend
Copy link
Author

@rubenvanassche this is ready to be re-reviewed 👍

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