[ditrack commit] r2407 - src/trunk/DITrack/DB

[Oleg Sharov]

Vlad Skvortsov пишет:
> Hey,
> 
> please make sure that the suggestions I've already made (quote deleted) 
> don't fall through cracks! Thanks!
> 
> Further comments inline.
> 
> 
> Шаров Олег wrote:
> 
> [skipped]
> 
>>>> +    def isset_new_comment(self, path):
>>>>       
>>> Comments?
>>>
>>> I would suggest renaming this function into is_last_firm_comment() 
>>> which would accept a firm comment number and tell if that's the 
>>> latest one.
>>>
>>>   
>> We have a firm comment number inside DITrack.DB.Issue.Issue, that is 
>> why we need isset_new_comment()
>> I suppose you didn't like this function, but I don't like the name 
>> which your suggest.
>> What do you think about isset_uncached_comment() ?
> 
> I think I see what you are saying about the comment number, though I 
> don't like either of the names you are suggesting (yeah, mine doesn't 
> work too ;-)). The real purpose of the method is to check whether the 
> in-memory representation of the issue is still valid with respect to the 
> working copy. Let's name it accordingly then! How about is_up_to_date() 
> or is_valid()?
> 
> The fact that we are checking the comment file existence is just an 
> implementation detail and shouldn't be exposed in the method name.
> 
> [skipped]
> 
> 
>>> Looking at the following block of code makes me thinking: should we 
>>> place all caching logic under Issue.load()? With the current approach 
>>> we have to be very careful to keep the cache coherent (e.g. whenever 
>>> the issue is loaded, we have to update the cache).
>>>
>>>   
>> Issue.load() can't  be used to work with cache, because class Issue is 
>> used for WC & LMA.
> 
> Yes, but the issue only gets load()ed from WC. Why exactly couldn't that 
> work?
> 

If we move work with cache to DB.Issue.load() we get cache files of each 
issues. Now we have one shelve for all issues. Besides, I'm thinking may 
be we must make DB.Issue more abstractive, and move logics of work with 
disk to DB.WC.

>>> ...or, maybe add a method to the WC class, say, load_issue() that 
>>> would transparently incorporate caching, and implement issues() in 
>>> terms of that?
>>>
>>>   
>> I doubt necessity of a new separate method.
> 
> See, there are currently two methods to load [an] issue(s) from the WC: 
> WorkingCopy.__getitem__() and WorkingCopy.issues(). The former "is not 
> aware" of the cache, which is clearly a bug. Both methods ultimately use 
> Issue.load() to read data from disk.
> 
> So, at the very least we need to factor out cache-related logic into a 
> separate (private) method and use that in both abovementioned places.
> 
>> But, I think we need to implement command to clear cache
>>
>> "dt cleanup" or "dt --clear-cache ls" or "dt --without-cache ls"
>>
>> because we can't predict all possible cases of cache clearing:
>> - delete one of comment manually
>> - delete one of issue manually
>> - etc
>>
>> Your opinion?
> 
> I think *for now* we can skip it. We can always recommend users to 
> remove the cache file manually and if people get annoyed by doing this, 
> we'll know that and implement a command. :-)
> 
> [skipped]
>