Cache date property values (fixes #384)

[eyobofficial]

Closes #384

[codecov[bot]]

Codecov Report

Merging #388 (f4e87b5) into master (017c7db) will increase coverage by 0.59%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #388      +/-   ##
==========================================
+ Coverage   97.19%   97.79%   +0.59%     
==========================================
  Files          20       20              
  Lines        1428     1453      +25     
==========================================
+ Hits         1388     1421      +33     
+ Misses         40       32       -8     

Impacted Files	Coverage Δ
enhydris/models.py	99.55% <100.00%> (+1.92%)	:arrow_up:

---

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 017c7db...f4e87b5. Read the comment docs.

[eyobofficial]

I think that @cached_property is not the right way to do it. This cache will persist only as far as that instance exists. After a request completes, the object no longer exists, and neither does its cached property. In the next request it will hit the database again.

So, for example, instead of this that you have in a test:

with self.assertNumQueries(0):
    self.timeseries_group.start_date

try this:

with self.assertNumQueries(0):
    models.TimeseriesGroup.get(id=self.timeseries_group.id).start_date

and it will probably fail, because you are creating a new instance.

I believe you need to use the Django cache explicitly, as we do with Timeseries.get_data().

Ok, I will make the changes accordingly.

[aptiko]

Merged in 70a82c88e11e1649e2a8282fd09bb3ed6a4033a2.

BTW, your name in this commit appears as "eyob tariku". Please capitalize it properly.

[eyobofficial]

Since this is great, we can waste some time and discuss about the very minor things :-)

You've used some functions in functions, like this:

@property
def last_update(self):
    def get_last_update():
        # ...

    cache.get_or_set("cache_key", get_last_update)

Here is how I'd write this:

@property
def last_update(self):
    cache.get_or_set("cache_key", self._get_last_update)

def _get_last_update(...):
    # ...

(Not certain whether it works, I haven't tested it.)

Although I avoid functions in functions, I understand how in this particular case they present the whole logic nicely. I'm thinking out loud here: According to Robert C. Martin, functions should be up to five lines large, which I try to do but I'm not very fanatic about it. So using a nested function makes the outside function already exceed that. If the inside function has the tendency to grow a bit, and you want to break it in more functions, you'll have to un-nest it, otherwise it will become too hard to read. So I think that my version is more general. I also think I prefer it because of the less indentation.

But you don't need to change it, I'm just exchanging views on clean code. I'm interested in your opinion.

@aptiko Even though I am still going through Robert C. Martin's book, I understand your point. I am also usually reluctant to use nested functions and my first instinct was to use separate private methods as you suggested. However, the 'inner' functions are only used by the 'parent' functions. Thus, I first tried to use the lambda expression as the callable argument of the get_or_set method, but that obviously didn't work since Python's lambda expression can only take one line of expressions. So, I finally decided to use the nested functions so as not to 'pollute' the class with methods that are only relevant for specific methods. However, this is not a pattern I expect to use more often.

[eyobofficial]

Since this is great, we can waste some time and discuss about the very minor things :-)

You've used some functions in functions, like this:

@property
def last_update(self):
    def get_last_update():
        # ...

    cache.get_or_set("cache_key", get_last_update)

Here is how I'd write this:

@property
def last_update(self):
    cache.get_or_set("cache_key", self._get_last_update)

def _get_last_update(...):
    # ...

(Not certain whether it works, I haven't tested it.)

Although I avoid functions in functions, I understand how in this particular case they present the whole logic nicely. I'm thinking out loud here: According to Robert C. Martin, functions should be up to five lines large, which I try to do but I'm not very fanatic about it. So using a nested function makes the outside function already exceed that. If the inside function has the tendency to grow a bit, and you want to break it in more functions, you'll have to un-nest it, otherwise it will become too hard to read. So I think that my version is more general. I also think I prefer it because of the less indentation.

But you don't need to change it, I'm just exchanging views on clean code. I'm interested in your opinion.

@aptiko Even though I am still going through Robert C. Martin's book, I understand your point. I am also usually reluctant to use nested functions and my first instinct was to use separate private methods as you suggested. However, the 'inner' functions are only used by the 'parent' functions. Thus, I first tried to use the lambda expression as the callable argument of the get_or_set method, but that obviously didn't work since Python's lambda expression can only take one line of expressions. So, I finally decided to use the nested functions so as not to 'pollute' the class with methods that are only relevant for specific methods. However, this is not a pattern I expect to use more often.