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

General code health improvements #920

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

Conversation

xHeaven
Copy link

@xHeaven xHeaven commented Dec 30, 2024

No description provided.

@xHeaven
Copy link
Author

xHeaven commented Dec 30, 2024

How do we feel about being very explicit with if conditions? We could merge things like this into a single if statement instead of branching out a bunch of them:

if (!is_object($value)) {
    return true;
}

if ($property->type->kind->isDataCollectable()) {
    return true; // Transform everything to data objects
}

// ---

$class = new ReflectionClass($className);

if (empty($class->getDocComment())) {
    return self::$cache[$className] = null;
}

if (! $this->isIterable($class)) {
    return self::$cache[$className] = null;
}

$type = $this->getCollectionReturnType($class);

if ($type === null || $type['valueType'] === null) {
    return self::$cache[$className] = null;
}

// ---

if ($property->type->isNullable || $property->type->isOptional) {
    return false;
}

if ($property->type->kind->isDataCollectable() && $rules->hasType(Present::class)) {
    return false;
}

if ($rules->hasType(BooleanType::class)) {
    return false;
}

if ($rules->hasType(Nullable::class)) {
    return false;
}

if ($rules->hasType(RequiringRule::class)) {
    return false;
}

@xHeaven
Copy link
Author

xHeaven commented Dec 30, 2024

I'm not entirely sure about b32a68d (#920), is this the intended behavior?
I've tested it. Take this code:

new \Spatie\LaravelData\Support\Validation\ValidationRuleFactory()->create("accepted_if:test,false")->__toString();

Original parser output:

"accepted_if:test,1" // incorrect

Modified parser output:

"accepted_if:test,false" // correct

@xHeaven
Copy link
Author

xHeaven commented Dec 30, 2024

For the failed test, I can either do this:

$collection = array_map(
    fn ($value) => $this->transformationClosure($nestedContext)($value),
    is_array($items) ? $items : $items->all()
);

Or I can revert the entire change to a foreach loop. Which one is preferred?

Edit: I went with my proposal.

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.

1 participant