Refactor CatListDisplayer, add some tests #288

klemens-st · 2018-01-09T20:56:10Z

I only wanted to refactor CatListDisplayer and create LcpWrapper but it quickly turned out that to do it properly I have to change many other files...

Anyway, I moved wrapping with tags and classes to LcpWrapper. This PR alters many lines of code but in most cases for clarity and ease of use, not much changes in how the plugin works, except:

something_tag and something_class parameters now work in all template functions, they take precedence over arguments
some template functions now have default parameter values to match simple shortcode bahaviour, e. g. get_category_link($tag = 'strong', $css_class = null)
I added thumbnail_tag because template function already had this feature but default build function didn't

Other thigs that don't change how things work but are important to mention:

Added complete tests for LcpWrapper
Added tests for CatListDisplayer but this is work in progress, test cases I wrote are a good start though, we need even more refactoring to write good tests for this class...

I edited the readme, but the incosistencies I mentioned in #284 are mostly in the wiki, especially the html & css section, I'll fix them once this is merged.

Fixes #282

klemens-st · 2018-02-24T08:54:51Z

Oh, I forgot I created a nice testing post on the vagrant machine for this. Sharing in case it makes things easier

catname
[catlist id=2 numberposts=2 catname=yes]

customfields
[catlist customfield_display='123,1234']

comments
[catlist tag="comments" comments=yes]

author
[catlist numberposts=2 author=yes author_class='a-class']

date
[catlist numberposts=2 date=yes date_class=oiu]

date modified
[catlist numberposts=2 date_modified=yes date_modified_tag=p date_modified_class=test]

morelink
[catlist id=2 numberposts=2 morelink="More"]

thumbnail
[catlist numberposts=10 thumbnail=yes thumbnail_size=thumbnail thumbnail_class=test]

posts morelink
[catlist numberposts=2 id=2 posts_morelink="link" posts_morelink_class="test"]

catlink
[catlist numberposts=3 id=2 catlink="yes" catlink_class='some-class' catlink_tag=p]

conditional title
[catlist numberposts=2 conditional_title="Test cond title" conditional_title_tag=h1 conditional_title_class=some-class]

template
[catlist template=pagination]

class
[catlist numberposts=2 class=some_class]

no_posts_text
[catlist id=923847293847293847 no_posts_text=nopostshere]

excerpt
[catlist numberposts=2 excerpt=yes]

protected
[catlist name="Codex" show_protected=no]

tags_as_class
[catlist tags_as_class=yes]

picandocodigo · 2018-02-24T10:00:38Z

@zymeth25 this is awesome! Can you please write me an e-mail?

klemens-st · 2018-02-24T10:01:13Z

Sure

picandocodigo · 2018-07-10T08:41:38Z

Bumping this just so I remember to review it and merge it.

klemens-st · 2018-07-10T08:55:11Z

When you merge #318 there will be a conflict here but I'll resolve it.

klemens-st · 2018-07-10T09:29:03Z

include/lcp-catlistdisplayer.php

-    endif;
-
-    return $this->assign_style($info, $tag);
+    return $this->wrapper->wrap($info, $tag, $css_class);
  }

  private function get_post_link($single, $text, $class = null){


I left get_post_link, get_post_title and lcp_title_limit as they are for now. Mostly because they work differently than the stuff above.

picandocodigo · 2020-06-16T22:58:31Z

Hey @zymeth25, not sure if you're still around doing any development, but I'm doing some refactoring in another branch and since I'm back on the PHP, it's a good moment to get the changes from this PR in. Really sorry for how long it's taken, but I had been doing very small updates to the plugin for a long while. Would you be able to update it at all or should I just go ahead and incorporate your changes into my branch? I'd like to at least have it rebased to remove the Merge branch 'master' into lcp-wrapper commits and make sure there are no conflicts. Thank you so much for all the work you've done for the plugin!

Cheers!

klemens-st · 2020-06-17T07:21:22Z

Hi, I haven't done much PHP work lately but I have some free time now for WordPress fun ;) I would have to re-read what I wrote in this PR because I don't remember the details but I see my past self was kind enough to leave some comments :D

This PR deals with display, have you added any display options according to the current procedure? If so, I'll have to refractor them too and include here.

picandocodigo · 2020-06-17T12:47:00Z

Hi Klemens, good to see you again!

I didn't refactor anything in the displayer yet, I can't remember what's changed since you opened this PR but I'm guessing not much. Apparently there are no conflicts with master, so that's good! I like what you did with LcpWrapper. That's what I'm trying to do here, extracting stuff to new classes/traits so it's easier to maintain/write tests, etc.

I think I might try to merge this PR into a new branch that incorporates the changes in the refactor branch that I am working on, to check if there are any conflicts. Or maybe just merge this one into master once we think it's good to go, and then rebase refactor with master.

No rush anyway, I'm trying to dedicate some time in the nights to this new refactoring, which should make things easier for us and anyone new to the project in the long run :)

klemens-st · 2020-06-17T20:01:38Z

Since it currently has no conflicts with the master branch it's probably a better idea to merge it with master. I remember I tested it thorougly and found no problems. I will have to set up my local testing env again to see if it's still good.

klemens-st · 2020-06-17T20:04:24Z

I'll rebase it to remove the merge commits.

codeclimate · 2020-06-17T21:51:42Z

include/lcp-wrapper.php

+   * @param string $css_class
+   * @return string
+   */
+  private function assign_style($info, $tag = null, $css_class = null){


Function assign_style has a Cognitive Complexity of 7 (exceeds 5 allowed). Consider refactoring.

codeclimate · 2020-06-17T21:51:45Z

Code Climate has analyzed commit db95501 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	3

View more on Code Climate.

klemens-st · 2020-06-17T21:56:09Z

Ok done. Travis is broken but all tests pass locally. As far as I'm concerned it's ready to be merged and all changes are pretty well documented/commented in the code or this PR.

picandocodigo · 2020-06-17T22:32:38Z

Merging this 🎉
I'm REALLY sorry it took over 2 years for me to review and merge it, this is amazing work and I really appreciate it!

Don't worry about the Code Climate comments at all, this PR fixes many issues and brings the displayer to a much better state.

The build is broken for Travis and me. I'll spend some time on fixing that after some refactoring. In the meantime I'm doing manual work to test stuff 😬

I'll rebase my branch and keep refactoring things out. One immediate thing I see here is we should exctract the template code into it's own class or trait! I'm going to start adding issues too so I can be more organized. Thanks again!!!

klemens-st · 2020-06-17T22:36:24Z

Nice :D Yes, the template thing is something I noticed when writing this. I have a separate branch where I started work on that. Will open a PR tomorrow so you can have a look. It's 2 years old but should be good, it was based on the lcp-wrapper branch.

picandocodigo · 2020-06-17T22:48:09Z

Ah awesome! Yeah, if it's based off of this, it should be fine 😄
I might work some more on this tomorrow night, if not Friday 👍

klemens-st changed the title ~~Refacor CatListDisplayer, add some tests~~ Refactor CatListDisplayer, add some tests Jan 9, 2018

klemens-st mentioned this pull request Feb 8, 2018

Why the category_description=yes is sometimes working - sometimes not? #282

Closed

klemens-st commented Jul 10, 2018

View reviewed changes

klemens-st mentioned this pull request Jul 20, 2018

Refactor LCP default params #326

Merged

klemens-st mentioned this pull request Aug 13, 2018

Fix args overwrite bug in exlude_tags #331

Merged

picandocodigo added this to the 1.0 milestone Sep 19, 2018

klemens-st mentioned this pull request Nov 12, 2018

content_getter() returns all types of content without leading space #360

Closed

klemens-st mentioned this pull request Nov 27, 2018

Styling category_description #364

Closed

picandocodigo force-pushed the master branch 8 times, most recently from 22a07a2 to 5efe57e Compare June 11, 2019 21:21

klemens-st added 4 commits June 17, 2020 22:55

Refactor tags and classes

435e685

Add tests for LcpWrapper

2d9fe5e

Add tests for LcpCatListDisplayer

9f17a24

Fix tests

db95501

klemens-st force-pushed the lcp-wrapper branch from 1a83872 to db95501 Compare June 17, 2020 21:51

codeclimate bot reviewed Jun 17, 2020

View reviewed changes

klemens-st mentioned this pull request Jun 17, 2020

Refactor #405

Merged

picandocodigo merged commit 7bc858a into picandocodigo:master Jun 17, 2020

picandocodigo mentioned this pull request Jun 17, 2020

Add @zymeth25's test post to the vagrant development blog #406

Open

klemens-st deleted the lcp-wrapper branch June 18, 2020 05:08

klemens-st mentioned this pull request Jul 1, 2020

Bug: parameters handling in CatListDisplayer::get_post_title #350

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor CatListDisplayer, add some tests #288

Refactor CatListDisplayer, add some tests #288

klemens-st commented Jan 9, 2018 •

edited

Loading

klemens-st commented Feb 24, 2018 •

edited

Loading

picandocodigo commented Feb 24, 2018

klemens-st commented Feb 24, 2018

picandocodigo commented Jul 10, 2018

klemens-st commented Jul 10, 2018

klemens-st Jul 10, 2018

picandocodigo commented Jun 16, 2020

klemens-st commented Jun 17, 2020 •

edited

Loading

picandocodigo commented Jun 17, 2020

klemens-st commented Jun 17, 2020

klemens-st commented Jun 17, 2020

codeclimate bot Jun 17, 2020

codeclimate bot commented Jun 17, 2020

klemens-st commented Jun 17, 2020 •

edited

Loading

picandocodigo commented Jun 17, 2020

klemens-st commented Jun 17, 2020

picandocodigo commented Jun 17, 2020

Refactor CatListDisplayer, add some tests #288

Refactor CatListDisplayer, add some tests #288

Conversation

klemens-st commented Jan 9, 2018 • edited Loading

klemens-st commented Feb 24, 2018 • edited Loading

picandocodigo commented Feb 24, 2018

klemens-st commented Feb 24, 2018

picandocodigo commented Jul 10, 2018

klemens-st commented Jul 10, 2018

klemens-st Jul 10, 2018

Choose a reason for hiding this comment

picandocodigo commented Jun 16, 2020

klemens-st commented Jun 17, 2020 • edited Loading

picandocodigo commented Jun 17, 2020

klemens-st commented Jun 17, 2020

klemens-st commented Jun 17, 2020

codeclimate bot Jun 17, 2020

Choose a reason for hiding this comment

codeclimate bot commented Jun 17, 2020

klemens-st commented Jun 17, 2020 • edited Loading

picandocodigo commented Jun 17, 2020

klemens-st commented Jun 17, 2020

picandocodigo commented Jun 17, 2020

klemens-st commented Jan 9, 2018 •

edited

Loading

klemens-st commented Feb 24, 2018 •

edited

Loading

klemens-st commented Jun 17, 2020 •

edited

Loading

klemens-st commented Jun 17, 2020 •

edited

Loading