Refactor get_title_cache to be straightforward #6829
Conversation
As well as correct during cache miss
There was a problem hiding this comment.
This review was actually done on Friday, the browser had cached it luckily. @jonathan-s
| return lang | ||
|
|
||
| if not language: | ||
| language = get_language() | ||
|
|
There was a problem hiding this comment.
get_fallback_language would have been called here if fallback is true and would have run before the force_reload, this could have unforeseen side affects. I can see that this condition is ran below with the use_fallbacks option, I'm not sure how that could affect being run later after the force reload.
There was a problem hiding this comment.
You are correct, you'll see the effects of this in the test that I provided. If you ran the test prior to this patch the test would fail. With this patch, the test passes.
|
|
||
| if fallback and not self.title_cache.get(language): | ||
| # language can be in the cache but might be an EmptyPageContent instance | ||
| def get_fallback_language(page, language): |
There was a problem hiding this comment.
makes sense, maybe as this is private it should be called: _get_fallback_language
There was a problem hiding this comment.
Was this created to attack it from versioning ?
There was a problem hiding this comment.
No, this method was made to reduce clutter. The same code was repeated twice. Also having a name get_fallback_language makes the code easier to parse than the block of code that does not have a name.
|
|
||
| def test_get_title_obj(self): | ||
| """ | ||
| Cache partially populated, if no hit it should try to populate it |
There was a problem hiding this comment.
There is a test cache test file, this should live in there. I thought that we had a test_title file, seems that we don't.
|
🎉 🎉 |
Prior to this patch if the cache is partially filled it would have used a fallback rather than trying to fill the cache with the correct pagecontent.
release/4.0.x