{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.