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

Namespace issues #43

Open
fknorn opened this issue Feb 17, 2024 · 3 comments
Open

Namespace issues #43

fknorn opened this issue Feb 17, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@fknorn
Copy link

fknorn commented Feb 17, 2024

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):

  • PHP Version: 8.3
  • CodeIgniter version: 4.4.5
  • Library version: 0.7.1
@fknorn fknorn added the bug Something isn't working label Feb 17, 2024
@hermawanramadhan
Copy link
Owner

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
Copy link
Author

fknorn commented Feb 17, 2024

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
Copy link
Owner

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants