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

[Шаров Олег]

Vlad Skvortsov wrote:
> oleg at ditrack.org wrote:
>  
>> Author: oleg
>> Date: 2007-12-07 11:49:11 -0800 (Fri, 07 Dec 2007)
>> New Revision: 2407
>>
>> Modified:
>>    src/trunk/DITrack/DB/Issue.py
>>    src/trunk/DITrack/DB/WC.py
>> Log:
>> added checking of new comment or issue
>>
>>
>> Modified: src/trunk/DITrack/DB/Issue.py
>> ===================================================================
>> --- src/trunk/DITrack/DB/Issue.py    2007-12-06 18:26:14 UTC (rev 2406)
>> +++ src/trunk/DITrack/DB/Issue.py    2007-12-07 19:49:11 UTC (rev 2407)
>> @@ -397,7 +397,16 @@
>>  
>>          return map(lambda k: "%s: %s%s" % (k, self.info[k], 
>> terminator), keys)
>>  
>> +    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() ?
>>  
>> +        n = int(self.firm_names[len(self.firm_names)-1])
>>     
>
> ...[-1]?
>
>  
>> +        fname = os.path.join(path, "comment%d" % (n + 1))
>> +
>> +        if os.path.isfile(fname):
>> +            return True
>>     
>
> Why not os.path.exists()? If we are checking if the name is really a 
> file, it makes sense to throw an exception if the entry exists but is 
> not in fact a file.
>
>  
>> +
>> +        return False
>> +
>>      def load(cls, path):
>>          """
>>          Load an issue from path PATH (should point to a directory).
>>
>> Modified: src/trunk/DITrack/DB/WC.py
>> ===================================================================
>> --- src/trunk/DITrack/DB/WC.py    2007-12-06 18:26:14 UTC (rev 2406)
>> +++ src/trunk/DITrack/DB/WC.py    2007-12-07 19:49:11 UTC (rev 2407)
>> @@ -261,25 +261,42 @@
>>          working copy.
>>          """
>>       
>
> 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.
> ...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.
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?
> These are just thoughts, I'm not really positive we need them at this 
> point (may be an 'XXX' in the code just to remember?). What do you think?
>  
>>  
>> +        res = []
>>          if len(self.cache):
>> -            return self.cache.get()
>>  
>> -        issue_re = re.compile("^i(\\d+)$")
>> +            issues = self.cache.get()
>>       
>
> Cache.__len__() does len(self.get()) and here we make another 
> redundant call to get(). Why not just do get() and see if anything is 
> returned?
>
>  
>> +            for id, issue in issues:
>> +                path = os.path.join(self.data_path, "i%s" % id)
>> +                if issue.isset_new_comment(path):
>> +                    issue = DITrack.DB.Issue.Issue.load(path)
>> +                res.append((id, issue))
>>  
>> -        lst = []
>> -        for fn in os.listdir(self.data_path):
>> -            m = issue_re.match(fn)
>> -            if m:
>> -                lst.append(int(m.group(1)))
>> +            while True:
>> +                id += 1
>> +                path = os.path.join(self.data_path, "i%s" % id)
>> +                if os.path.isdir(path):
>> +                    issue = DITrack.DB.Issue.Issue.load(path)
>> +                    res.append((id, issue))
>> +                else:
>> +                    break
>>  
>> -        lst.sort()
>> +        else:
>>  
>> -        res = []
>> -        for id in lst:
>> -            path = os.path.join(self.data_path, "i%s" % id)
>> -            issue = DITrack.DB.Issue.Issue.load(path)
>> -            res.append((id, issue))
>> +            issue_re = re.compile("^i(\\d+)$")
>>  
>> +            lst = []
>> +            for fn in os.listdir(self.data_path):
>> +                m = issue_re.match(fn)
>> +                if m:
>> +                    lst.append(int(m.group(1)))
>> +
>> +            lst.sort()
>> +
>> +            for id in lst:
>> +                path = os.path.join(self.data_path, "i%s" % id)
>> +                issue = DITrack.DB.Issue.Issue.load(path)
>> +                res.append((id, issue))
>> +
>>          self.cache.set(res)
>>  
>>          return res
>>
>> _______________________________________________
>> Commit mailing list
>> Commit at lists.ditrack.org
>> http://lists.ditrack.org/mailman/listinfo/commit
>>       
>
>