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

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

Issue.load() can't be used to work with cache, because class Issue is
used for WC & LMA.

Post by Vlad Skvortsov
...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?

Post by Vlad Skvortsov
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 = []
- 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?

Vlad Skvortsov

2008-04-26 00:02:09 UTC

Hey,

please make sure that the suggestions I've already made (quote deleted)
don't fall through cracks! Thanks!

Further comments inline.

Шаров Олег wrote:

[skipped]

Post by Vlad Skvortsov
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]

Post by Vlad Skvortsov
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?

Post by Vlad Skvortsov
...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.

Post by Ð¨Ð°ÑÐ¾Ð² ÐÐ»ÐµÐ³
But, I think we need to implement command to clear cache
"dt cleanup" or "dt --clear-cache ls" or "dt --without-cache ls"
- 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]

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

Ivan Glushkov

2008-05-05 14:16:38 UTC

Hi.

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

I'd like to vote to the Oleg's proposition. Let's compare the
following two methods of clearing cache:
- 'rm issues/.ditrack/.cache'
do the user always remember where this cache-file is located? I
suppose no. How should it recall it? 'grep cache -r *' (hoping that we
mentioned it in some README file)
- 'dt cleanup'
it's the usual interface with the user, so it'll always get help
by 'dt --help'.

Ivan.

Vlad Skvortsov

2008-05-12 23:38:45 UTC

Post by Ivan Glushkov

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

I'd like to vote to the Oleg's proposition. Let's compare the
- 'rm issues/.ditrack/.cache'
do the user always remember where this cache-file is located? I
suppose no. How should it recall it? 'grep cache -r *' (hoping that we
mentioned it in some README file)
- 'dt cleanup'
it's the usual interface with the user, so it'll always get help
by 'dt --help'.

I still stand by my opinion that there is no need to add this complexity
right away. We do have a mechanism for automatic cache repopulation so
for %99.99 cases of regular usage there should be no need to even think
about the cache. When things get hosed, there is always an option of
just removing the working copy and checking it out afresh.

So I think there is no need to consider adding new commands at least
until we see user requests/complaints.

--
Vlad Skvortsov, vss-***@public.gmane.org, http://vss.73rus.com

Oleg Sharov

2008-05-11 14:14:39 UTC

Post by Vlad Skvortsov
Hey,
please make sure that the suggestions I've already made (quote deleted)
don't fall through cracks! Thanks!
Further comments inline.
[skipped]

Post by Vlad Skvortsov
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]

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.

Post by Vlad Skvortsov
...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.

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.

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]

Vlad Skvortsov

2008-05-21 02:25:45 UTC

Oleg Sharov wrote:

[skipped]

Post by Oleg Sharov