Add Conditionable traits & phpdoc

[gwleuverink]

This pull request adds the Conditionable trait to all Facades where it makes sense (omitted all that don't use fluent chaining)

• Added the Conditionable trait to the following classes, allowing the use of when and unless methods for more expressive and concise conditional logic:
  • App
  • Alert
  • AutoUpdater
  • ChildProcess
  • Clipboard
  • Dock
  • Notification

Facade method annotations:

• Updated Facade classes to include @method annotations for when and unless, ensuring IDEs and static analysis tools are aware of the new methods:
  • App
  • Alert
  • AutoUpdater
  • ChildProcess
  • Clipboard
  • Dock
  • Menu already conditionable, not annotated
  • Notification

[PeteBishwhip]

Looks good. Worth a test to verify and assert the functionality remains and doesn't regress? :)

[gwleuverink]

Sure 👍🏻 What sort of test do you have in mind?

I'm only dropping in a trait here that is already covered by Laravel. So you must be meaning to test the integration with the existing logic?

We can't do a full end-to-end test with Electron for this. We can do assertions on a mock object to see if methods were called, but I don't think those will be very valuable. Do you have any suggestions on how to cover regressions @PeteBishwhip ? Keen to know your vantage point

[PeteBishwhip]

To me, I think some tests don't have to test functionality as much as ensure certain traits.

For example, I have a test in my own repos which simply make sure that all classes in a specific namespace extend a specific class and fails if one doesn't.

In this case, if we can dynamically pull a list of registered facades, iterate through and ensure they include the Conditional trait... Or even just iterate the directory of Facades and just verify use Conditional; is present.

Just a small little failsafe that ensures we remain consistent. I'm happy to pop in a test for it on this PR if you're happy for me to? :)

[gwleuverink]

Ah check I get what you're saying! Perfect 👌 no worries I'm already at it

[simonhamp]

Beautiful! 😍

[gwleuverink]

@PeteBishwhip Is this what you had in mind?

800f055

[gwleuverink]

Hmm the test don't pass with prefer-lowest. Must be a incompatibility with the architecture testing api 🤔

Method "toUseTrait" does not exist in string.

pestphp/pest: 2.30.0
pestphp/pest-plugin: 2.1.1
pestphp/pest-plugin-arch: 2.5.0

[PeteBishwhip]

@gwleuverink - That would work but doesn't protect against future additions. My thoughts were more to do a full scan of src/Facades and iterate through all, ensuring that the implement Conditionable.

This way, we can explicitly exclude a class if it should not include it, but by default, the test expects all and any newly created Facades to include it.

Something like:
tests/Pest.php:

use Symfony\Component\Finder\Finder;

/**
 * Get all concrete classes from facades in a namespace
 *
 * @param string $namespace The namespace to search (e.g., 'Native\Desktop\Facades')
 * @param string $path The directory path to search
 * @return Collection Collection of concrete class names from getFacadeAccessor()
 */
function getClassesInNamespace(string $namespace, string $path): Collection
{
    if (!is_dir($path)) {
        return [];
    }

    $classes = [];
    $finder = Finder::create()->files()->name('*.php')->in($path);

    foreach ($finder as $file) {
        $facadeClass = $namespace . '\\' . $file->getBasename('.php');
        
        if (class_exists($facadeClass) && method_exists($facadeClass, 'getFacadeAccessor')) {
            $concreteClass = $facadeClass::getFacadeAccessor();
            
            if (is_string($concreteClass) && class_exists($concreteClass)) {
                $classes[] = $concreteClass;
            }
        }
    }

    return collect($classes);
}

The test:

use Illuminate\Support\Traits\Conditionable;

it('ensures all facade concrete classes use Conditionable trait', function () {
    $excluded = [
        // Add explicitly excluded classes here
    ];

    arch()
        ->expect(
            getClassesInNamespace(
                'Native\\Desktop\\Facades',
                __DIR__ . '/../../src/Facades' // From tests/Architecture
            )->filter(fn ($value) => !in_array($value, $excluded))->toArray()
        )
        ->each
        ->toUseTrait(Conditionable::class);
});

I haven't tested this, and I took ages typing it in the comments box because I'm silly but it should work. Whats your thoughts?

[gwleuverink]

I see what you mean. Sure I can do that for the facades.

Not all of these in the list are facades though, I'll keep the others hardcoded.

[gwleuverink]

The test was failing on prefer-lowest when using pest 2.

I admit that this workaround is a bit janky. But if this runs on the higher versions i think that is enough.

[PeteBishwhip]

My understanding (albeit wrong looking back) was that we were adding Conditionable to all Facade insert faceplam emoji.

If it's specific ones, not really a way to dynamically ensure that things include it. In that case, your solution is okay.

[gwleuverink]

Most of these classes also have a facade so what you propose would work! We only also have to mix in some additional classes 👍 let me see if I can make it work

[gwleuverink]

Okay @PeteBishwhip i came to the same conclusion after writing it. I'll have to exclude about the same amount of classes I could remove using the 'dynamic' method. Maybe as it is is best in retrospect

[PeteBishwhip]

Okay, no biggie. Was hoping for something to ensure standards moving forward but perhaps thats a project for the future :)

[gwleuverink]

I'm on board with the idea. It's just that about only half of these facades have a fluent api where the Conditionable trait makes sense. When I had the solution ready and done I reconsidered your previous comment 😅