[spambayes-dev] Experimental SpamBayes build available

[Mark Hammond]

{Tony]
> > I really don't like the code in Options.py that handles the
> > default values for these storage items.  I'm not sure it is
> > to blame, but it did cause me to see a new .db file created
> > in the cwd, rather than the data directory - as my INI file
> > already existed, it didn't get the default FQN for the new option.
>
> The idea was that if you already had an ini file, then you
> had already set
> things up (you were an existing user), and so we wouldn't
> want to fiddle
> with your setup, whatever it was, because that might mean
> that we lose track
> of your databases.

That sounds like a worthy goal, but I don't see how the existing code solves
it.  Indeed, whenever we add a new option, things go quite wrong - the INI
file exists, so the new option is not written to an existing user's ini
file - hence, a semi-"ramdom" file is chosen (where the randomness comes
from whatever the CWD happens to be).

> (This could even be with the default "hammie.db" in the
> cwd for persistent_storage_file, if the user always runs the
> script from the
> same directory).

Doesn't that still work?  Even when there is no INI file on disk, the INI
file has a logical path - the one where we would write it when asked - so
the same concept still applies.

> Part of the problem is that when consolidating the storage
> name options, I
> picked "hammie.db" over "~/.hammiedb", which was by far the
> better option.

I think here you are only deciding between making the default for a specific
option a relative or absolute filename (where ~/whatever obviously expands
absolutely).  The scheme still works regardless of the decision made for a
specific option.

> I didn't realise that it would expand quite nicely on Windows
> (well, 2k and
> XP; I presume earlier as well), and I presume on Macs as well.

It "works" on Windows, but by default is unlikely to give you the directory
you expect.  Cygwin users, for example, are likely to have it expand to an
absolute path, but not the one where SpamBayes should store its INI file.
On Windows, HOME is likely to be set to satisfy the most valuable, but
braindead, Windows port of a Unix app is used <wink>.

> The proper default, of course, is still "hammie.db".

Actually, the 'of course' was not at all obvious to me.  I assumed the
default was still going to be 'statistics_database.db' - I hadn't considered
the possibility the default value would be changed, but assumed only the
paths were being mangled.

>  When I
> put that code
> in to put things into a better place for Windows users I
> figured that since
> they wouldn't have an existing db, a more easily
> understandable name would
> be good, too, but I didn't think that I could change it for
> everyone.  Do we
> continue setting this option to "statistics_database.db" in
> that place? (Without the FQN).

-1 - Clearly the new name is not better just for 'windows users', so Windows
should get no special treatment.  If the new name is truly better for
everyone, then it should be changed.

It just makes the code more complex for absolutely no benefit, and as I
mentioned above, has already seriously misled me.

> The same code also gives new default
> names for the cache
> directories and messageinfo db.

Again, -1.  The only thing special here for Windows users is 'the default
data directory', so the code unique to Windows should deal only with this
issue.

Had I noticed the changing of the default values, I certainly would have
included that in my list of things to remove <wink>

> What about these?
>   [TestDriver] spam_directories
>   [TestDriver] ham_directories

Assuming no special casing of these options for Windows <wink>, the current
default appears to be a relative path name.  I see no good reason to
continue to allow these to be relative to the cwd rather than relative to
the INI file used to control the tests.  If there is a clear reason I
missed, just express them as comments where the call is *not* made <wink>

> What about this?

I'd be inclined to avoid the expanduser() on Windows.  Either skip the call
completely, or special case it to merge in our default data directory.  I
really don't see how the special casing would be used by more than a handful
of geeks, so I reckon you should just skip it.

> How do people feel about having this happen implictly when
> one of these
> options is used, rather than explicitly?  (I worry that we'll miss an
> occurrence of one of them, or that someone (maybe me!) will
> add new code and
> forget to use the get_pathname_option function).  Something like this:
>
> [Current code in OptionsClass.py]
>     def get(self):
>         '''Get option value.'''
>         return self.value
>
> [Proposed]
>     def expand_path(value):
>         filename = os.path.expanduser(value)
>         if os.path.isabs(filename):
>             return filename
>         return os.path.join(os.path.dirname(optionsPathname),
> # existing
> global

Certainly don't want to use a module global here.

I'm -0 on this, unless the proposed patch really would be much better and
clearer with new magic.  Testing will show up failure to call this function
pretty quickly, as the file will be created in the CWD - exactly the issue I
had, which lead us to this point <wink>

Mark.