[MERGE] per-user file properties
Martin Pool
mbp at canonical.com
Tue May 13 05:10:26 BST 2008
Ian asked me to review his current branch adding glob-based properties.
I've given most of these comments in person as we're working together in
Brisbane today, but I'll post them here for visibility.
My main comment is that I don't think 'properties' is a very good name for
them. It implies that they are, attached to or contained within the
particular file, and this is reinforced by very prominent use this way in
Subversion.
Really this is something more like 'categories', 'rules' or or in git's
terms 'attributes', or maybe we can think of a better name. We should
rename the config file name as well as the internals.
=== modified file 'bzrlib/globbing.py'
--- bzrlib/globbing.py 2007-03-07 14:49:00 +0000
+++ bzrlib/globbing.py 2008-05-13 02:09:19 +0000
@@ -214,7 +214,29 @@
if match:
return patterns[match.lastindex -1]
return None
-
+
+
+class _OrderedGlobster(Globster):
+ """A Globster that keeps pattern order."""
It might be good to add a doctest, to give an example of how it works.
You will need to specifically add this file as using doctests.
=== added file 'bzrlib/properties.py'
--- bzrlib/properties.py 1970-01-01 00:00:00 +0000
+++ bzrlib/properties.py 2008-05-13 02:09:19 +0000
+
+from bzrlib import (
+ config,
+ globbing,
+ osutils,
+ )
+from bzrlib.util.configobj import configobj
+
+
+class _PropertiesProvider(object):
+ """An object that provides properties for a file."""
The docstring is a bit vague. How about:
Look up per-entry rules from a file by matching globs.
It might be good for the class name to say that it's from a file in
particular. If this is later done on a RevisionTree then it will be given
its input as a byte string or stream, rather than as a file with a
filename, and perhaps it should be done that way. (Maybe yagni.)
+# The object providing per-user properties
+_user_properties_provider = _PropertiesProvider(properties_filename())
This seems to not actually be used yet. The best entry point to get them
is probably something like
Tree.get_entry_rules(file_names, attribute_names)
I think the general pattern is that most methods like this should be able
to act on several files at a time rather than being called for each one;
doing so seems to have some potential for speeding up the search.
Also with a view to performance: we previously added automatic read_lock
decorators on many methods, but I think are more recently deciding that
doing this automatically is an unnecessary performance burden. So I'd
suggest when adding this new API it should require (and document) that the
caller must hold a read (or write) lock.
=== added file 'bzrlib/tests/test_properties.py'
--- bzrlib/tests/test_properties.py 1970-01-01 00:00:00 +0000
+++ bzrlib/tests/test_properties.py 2008-05-13 02:09:21 +0000
+ def setUp(self):
+ super(TestPropertiesPath, self).setUp()
+ os.environ['HOME'] = '/home/bogus'
+ if sys.platform == 'win32':
+ os.environ['BZR_HOME'] = \
+ r'C:\Documents and Settings\bogus\Application Data'
+ self.bzr_home = \
+ 'C:/Documents and Settings/bogus/Application Data/bazaar/2.0'
+ else:
+ self.bzr_home = '/home/bogus/.bazaar'
It seems like this should be done in a base class, if it is not already.
The setting of HOME should be next to the last line.
+
+ def test_properties_filename(self):
+ self.assertEqual(properties.properties_filename(),
+ self.bzr_home + '/fileproperties')
+
+
+class TestPropertiesProvider(tests.TestCase):
+
+ def make_provider(self, lines):
+ """Make a _PropertiesProvider from a list of strings"""
+ # This works even though the API doesn't document it yet
+ return properties._PropertiesProvider(lines)
... because ConfigObj can take a list of lines.
Otherwise good.
More information about the bazaar
mailing list