search

bgultekin / laravel4-datatables-package

Server-side handler of DataTables Jquery Plugin for Laravel 4
267 stars 108 forks source link

Discussion, internal code structure #150

Open phazei opened 10 years ago

phazei commented 10 years ago

Not sure where discourse for the plugin might go. But since you're down with my pull requests I had some suggestions.

Currently it's ver 1.4.4. Also the install instructions say to install it with: "bllim/datatables": "*"

It might not be a bad idea to change the install instructions to: "bllim/datatables": "1.4.*" and move some of the new modifications to a 1.5 tag.

Also, the names and descriptions of many of the internal methods aren't entirely clear, and some are camelCase while most other's are underscored. It seems to be the laravel convention for camelCase. Don't know what you'd think about me organizing that a bit more. According to packagist, a shit ton of people are using this and I wouldn't want to jack anything up.

MarkVaughn commented 10 years ago

we could move all functions to one style, I prefer camelCase :+1: , have the old ones of the other style have an @deprecated in the docs

Mark Vaughn Mobile: +1 (770) 533-2274

On Wed, Sep 17, 2014 at 5:20 PM, phazei notifications@github.com wrote:

Not sure where discourse for the plugin might go. But since you're down with my pull requests I had some suggestions.

Currently it's ver 1.4.4. Also the install instructions say to install it with: "bllim/datatables": "*"

It might not be a bad idea to change the install instructions to: "bllim/datatables": "1.4.*" and move some of the new modifications to a 1.5 tag.

Also, the names and descriptions of many of the internal methods aren't entirely clear, and some are camelCase while most other's are underscored. It seems to be the laravel convention for camelCase. Don't know what you'd think about me organizing that a bit more. According to packagist, a shit ton of people are using this and I wouldn't want to jack anything up.

— Reply to this email directly or view it on GitHub https://github.com/bllim/laravel4-datatables-package/issues/150.

phazei commented 10 years ago

Actually, there's already a magic __call method from PR #93 to allow calling via camelCase. So it can internally be changed to camelCase, and that magic method can just be switched to allow the call via snake case. There's actually a laravel snake_case helper function to do that.

ktunkiewicz commented 10 years ago

Actually, there's already a magic __call method from PR #93 to allow calling via camelCase.

I added the magic method because, like you mentioned this seems to be laravel convention, but didn't want to break backwards compatibility. Rewriting original function names to snakeCase sound good to me.

There's actually a laravel snake_case helper function to do that.

The snake_case works oposite way, camel_case function would be a solution here

According to packagist, a shit ton of people are using this and I wouldn't want to jack anything up.

Changing internal (protected) functions names should not break anything until somebody extends this class with his own subclass... But I don't think that would be many people doing this. What do you think about it Mark?

It might not be a bad idea to change the install instructions to: "bllim/datatables": "1.4.*" and move some of the new modifications to a 1.5 tag.

Why is that a better idea than using * ? Advicing people to use asterix in version name causes more people to use current version of script and response faster for code changes / bugs.

Anyway we should release 1.5 version soon as there were some important changes to mData support that can be concidered as a completly new feature. But please write some Readme notes about the changes you made Phazei, we shouldn't introduce new features without usage instructions :)

phazei commented 10 years ago

I'll be able to update the readme in a few days. Have a project due and been spending way to much time on this.

I've done some work on the prefix column. I'm not entirely sure of it's necessity or that of all the DB::raw stuff. I was looking at issue #26 and some of those issues awsp mentioned are still there.

There's a lot of discrepancies between the global search and column search too.

I was wondering about on global search, it's doing a search for "(" and if it exists, it uses raw. I guess that's specifically for Expressions. Shouldn't it just check if it's an expression? It couldn't have a ( otherwise I don't think. I traced it down to issue #66 for more info but that was more about aliasing.

ktunkiewicz commented 10 years ago

There is a TODO issue #122 - I made it to organize future development as there were some bugs that were not urgent to fix and looked for me like a bigger work to do (like #106 or #26)

I've done some work on the prefix column. I'm not entirely sure of it's necessity or that of all the DB::raw stuff. I was looking at issue #26 and some of those issues awsp mentioned are still there.

I didn't dig into that yet...

There's a lot of discrepancies between the global search and column search too.

Can you give some examples?

was wondering about on global search, it's doing a search for "(" and if it exists, it uses raw. I guess that's specifically for Expressions. Shouldn't it just check if it's an expression?

I think that clean_columns function may cast the DB::raw to string so it is not DB::raw anymore... This is why the expression is recognized by searching for ( butr of course this could be done better by changing clean_columns function not to cast DB::raw to string.

ktunkiewicz commented 10 years ago

Later at home I will take a deeper look into all the changes you made, theres lots of it, and most of there changes seems very good so I think its time for me to put some more attention on developing this plugin and test it together with you.