uxweb / sweet-alert

A simple PHP package to show SweetAlerts with the Laravel Framework
MIT License
834 stars 209 forks source link

Unnecessary function in tests #92

Closed meijdenmedia closed 4 years ago

meijdenmedia commented 5 years ago

Since a few days I'm trying out Scrutinizer. When I add a call to get something from my Laravel config files, Scrutinizer gives me an error.

It tells me my config() method should implement a second parameter. The second parameter for config() isn't mandatory, so I think it is picking the wrong config method.

Why do we need an config() method in this test file? Can we move it inside a namespace to avoid double methods? Or can it be removed?

https://github.com/uxweb/sweet-alert/blob/master/tests/SweetAlertNotifierTest.php#L369

Screenshot 2019-05-06 at 14 53 28

uxweb commented 5 years ago

Are you running that tool even on all the vendor code? On May 6, 2019, 7:54 AM -0500, MeijdenMedia notifications@github.com, wrote:

Since a few days I'm trying out Scrutinizer. When I add a call to get something from my Laravel config files, Scrutinizer gives me an error. It tells me my config() method should implement a second parameter. The second parameter for config() isn't mandatory, so I think it is picking the wrong config method. Why do we need an config() method in this test file? Can we move it inside a namespace to avoid double methods? Or can it be removed? https://github.com/uxweb/sweet-alert/blob/master/tests/SweetAlertNotifierTest.php#L369 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

meijdenmedia commented 5 years ago

Our codebase is being installed on a VM and all vendors are installed via composer install, this is default behaviour for Scrutinizer. I think this is because they want to check our codebase for correct calls to external packages: e.g. missing function parameters, incorrect parameter types, stuff like that.

meijdenmedia commented 5 years ago

Is it possible to move this call? Or at least make the parameters equal to the Laravel method? I can create a PR for that if you want to.

The method in the Laravel helpers:

if (! function_exists('config')) {
    /**
     * Get / set the specified configuration value.
     *
     * If an array is passed as the key, we will assume you want to set an array of values.
     *
     * @param  array|string  $key
     * @param  mixed  $default
     * @return mixed|\Illuminate\Config\Repository
     */
    function config($key = null, $default = null)
    {
        if (is_null($key)) {
            return app('config');
        }

        if (is_array($key)) {
            return app('config')->set($key);
        }

        return app('config')->get($key, $default);
    }
}
uxweb commented 5 years ago

@meijdenmedia

That call is for mocking the Laravel config in tests, it is not actually used in production code.