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

[Vlad Skvortsov]

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.

>  
> +        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).

...or, maybe add a method to the WC class, say, load_issue() that would 
transparently incorporate caching, and implement issues() in terms of that?

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
>   


-- 
Vlad Skvortsov, vss at 73rus.com, http://vss.73rus.com