Skip to content

Streamline get_meta_values functions across objects#2019

Merged
jarednova merged 4 commits into2.xfrom
2.x-meta-get-meta-values
Jun 11, 2019
Merged

Streamline get_meta_values functions across objects#2019
jarednova merged 4 commits into2.xfrom
2.x-meta-get-meta-values

Conversation

@gchtr
Copy link
Copy Markdown
Member

@gchtr gchtr commented May 31, 2019

Ticket: #1617

Issue

There were failing tests after merging in #2014. And it made me realize that for the meta functionality, I added way too few tests.

Solution

This pull request first streamlines the functionality of what the get_meta_values() does across Post, Term, User and Comment objects. I figured it’s actually good this is coming together step by step, because when I started out with working on meta for 2.x, I was hard to get an overview.

Now, all objects import all meta values into a custom property. And all get_meta_values() functions do pretty much the same, including the same variable pattern etc.

I also added tests for the different objects. The tests that were introduced in #2012 are really good to test the timber/{object}/pre_get_meta_values filter (Thanks, @aj-adl!).

Impact

Fewer fails.

Usage Changes

None.

Considerations

I need to add more tests to improve coverage of changes for meta. There will be more pull requests.

Testing

Yes.

@gchtr gchtr added the 2.0 label May 31, 2019
@codecov
Copy link
Copy Markdown

codecov bot commented May 31, 2019

Codecov Report

❗ No coverage uploaded for pull request base (2.x@70ff3c9). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##             2.x    #2019   +/-   ##
======================================
  Coverage       ?   95.36%           
  Complexity     ?     1507           
======================================
  Files          ?       50           
  Lines          ?     3865           
  Branches       ?        0           
======================================
  Hits           ?     3686           
  Misses         ?      179           
  Partials       ?        0
Impacted Files Coverage Δ Complexity Δ
lib/Term.php 98.8% <100%> (ø) 54 <0> (?)
lib/Post.php 97.79% <100%> (ø) 198 <0> (?)
lib/User.php 97.24% <100%> (ø) 37 <0> (?)
lib/Comment.php 96% <100%> (ø) 60 <0> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70ff3c9...d3ac54d. Read the comment docs.

@gchtr gchtr added the Ready for Review Ready for a contrib to take a look at and review/merge label May 31, 2019
@gchtr gchtr mentioned this pull request Jun 4, 2019
@gchtr gchtr requested a review from jarednova June 10, 2019 19:26
@gchtr
Copy link
Copy Markdown
Member Author

gchtr commented Jun 10, 2019

@jarednova This pull request should fix the current build issues in 2.x.

@jarednova
Copy link
Copy Markdown
Member

Great! Will review so we can unblock the other 2x stuff

Copy link
Copy Markdown
Member

@jarednova jarednova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a few touch-ups to bring these more into line across the classes, namely:

  • Specify array as the return type
  • Same docblock for args
  • Have all functions handle the post/comment/term/user ID in the same way

@jarednova jarednova merged commit 23a3f27 into 2.x Jun 11, 2019
@jarednova jarednova deleted the 2.x-meta-get-meta-values branch June 11, 2019 13:47
@gchtr
Copy link
Copy Markdown
Member Author

gchtr commented Jun 11, 2019

@jarednova Cool, thanks! 👍 I feel like the more we iterate with small changes, the more we see the patterns that will make different object types work in the same way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.0 Ready for Review Ready for a contrib to take a look at and review/merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants