hermawanramadhan / CodeIgniter4-DataTables

MIT License
92 stars 38 forks source link

Namespace issues #43

Open fknorn opened 6 months ago

fknorn commented 6 months ago

Describe the bug In your code and contained docblocks you use e.g. (from file DataTable.php)


    namespace Hermawan\DataTables;

    /**
     * Make a DataTable instance from builder.
     *  
     * Builder from CodeIgniter Query Builder
     * @param  Builder $builder
     */
    public static function of($builder)
    {
        return new self($builder);
    }

    /**
     * Add Searchable columns
     * @param String|Array
     */
    public function addSearchableColumns($columns)
    {
        $this->columnDefs->addSearchable($columns);
        return $this;
    }

This means that the in the static of function, the $builder argument is considered to be of type Hermawan\DataTables\Builder.

In the function addSearchableColumns, the $columns argument is not a regular, native php string (as you probably intend) but a Hermawan\DataTables\String.

Etc. etc.

However, since the builder is generated based off Codeigniter code, it is in fact of type CodeIgniter\Database\BaseBuilder. And what we pass to addSearchableColumns is a native string, etc.

This discrepancy is highlighted by my code linter all the time, all over the place and I believe it should be fixed. The solution would be, continuing with the example at from the top:


    namespace Hermawan\DataTables;

    /**
     * Make a DataTable instance from builder.
     *  
     * Builder from CodeIgniter Query Builder
     * @param  \CodeIgniter\Database\BaseBuilder $builder        // <<<< note the change
     */
    public static function of($builder)
    {
        return new self($builder);
    }

    /**
     * Add Searchable columns
     * @param string|array                                   // <<<< note the lowercase
     */
    public function addSearchableColumns($columns)
    {
        $this->columnDefs->addSearchable($columns);
        return $this;
    }

I'm no expert in namespace, but I believe this would be more correct? It stops all the linter errors in any case...

Thanks!

Version (please complete the following information):

hermawanramadhan commented 6 months ago

maybe you're right, I dont really understand when I wrote this code. is declaration type data on the comment like this will affect?

fknorn commented 6 months ago

Exactly, it's what's written in the comment (docblock) where the linter takes the information from. Ideally it should be in the function argument too, to make it strict.

E.g.


    namespace Hermawan\DataTables;

    /**
     * Make a DataTable instance from builder.
     *  
     * Builder from CodeIgniter Query Builder
     * @param  \CodeIgniter\Database\BaseBuilder $builder
     */
    public static function of(\CodeIgniter\Database\BaseBuilder $builder)
    {
        return new self($builder);
    }

    /**
     * Add Searchable columns
     * @param string|array
     */
    public function addSearchableColumns(string|array $columns)
    {
        $this->columnDefs->addSearchable($columns);
        return $this;
    }
hermawanramadhan commented 6 months ago

yes, phpdoc-types I mean..

about argument type declaration I knew it. but I only afraid of compatibility with old php version..

btw.. Thanks for sharing!