General refactoring

[klemens-st]

Hi @picandocodigo , hope all is well in the new year 😉 Following what you said over two weeks ago:

I've been meaning to refactor stuff for a while, as I did with the paginator, so it's easier to test.

I'd like to focus on refactoring and writing tests before I send any other enhancements I've been thinking about. I'm opening this thread so that we can agree on what to do and how: I know you are refactoring stuff yourself so I don't wont to unnecessarily double your work.

To start, I'm planning to:

• [x] write tests for LcpUtils once you have reviewed #283
• [x] refactor LcpCatListDisplayer so that all the wrapping in [whatever]_tag and [whatever]_class is done in a speparate class, write a test suit for this
• [x] write tests for LcpCategory
• [x] make sure all functionality of LcpCategory works as intended
• [x] Use templates instead of default build method
• [x] write tests for get_post_title()
• [x] Refactor HTML generation

(this list is meant to be edited as things are done or new ideas appear)

[picandocodigo]

Hi @zymeth25, happy new year! 🎉 I'll review the new PR after work today but it looks good so far. As you already saw, I started using static functions for LcpUtils but I was too lazy to change the older functions 😝

That class in particular should be rather easy to test compared to others. LcpCatListDisplayer needs lots of refactoring, moving the tag and class functionality to a separate class is a great start. I'll start adding tests myself too.

Thanks for all the hard work you've been doing for the plugin!

[klemens-st]

Yes, I think using static functions in LcpUtils works much better 👍

[klemens-st]

Looking at tags and classes now, the code is a bit inconsistent with what's written in the wiki, for instatnce the default tag for customfield_display is div not span. I guess I shouldn't try to unify it becuase it would break backward compatibility. But I'll edit the docs so that all exceptions are documented.

[klemens-st]

@picandocodigo I'm working on CatListDisplayer, refactoring and moving tag/class wrapping to a separate class. I noticed something I'd like to discuss with you because it requires a decision to be made to solve inconsistencies.

So when we are not using templates everything is fine, but when using templates we have 3 different scenarios regarding handling something_tag and something_class shortcode parameters:

• shortcode parameters are ignored and only $tag and $css_class arguments passed to template functions are considered
• shortcode parameters take precedence over template function arguments
• function arguments take precedence over shortcode parameters

Different methods of this class have different behavior, one of those 3. I think this really should be unified. My suggestion is the second option, shortcode params take precedence over function arguments.

Once this is decided I'll finish my work and submit a PR.

[picandocodigo]

for instatnce the default tag for customfield_display is div not span. I guess I shouldn't try to unify it becuase it would break backward compatibility. But I'll edit the docs so that all exceptions are documented.

Yes, my intention in general was to have divs as default, but I might have missed a few. If there's any specific inconsistency that's too different from the rest, we could fix it and just add a message on the changelog and readme.

I agree with the different scenarios for the template too, parameters should take precedence. We should just document it and go that way. Thanks for all your work!