[MERGE] Rule-based preferences (EOL part 1 of 3)

[John Arbash Meinel]

John Arbash Meinel has voted tweak.
Status is now: Conditionally approved
Comment:
Do we know that regexes always match the first item that can match? I 
have the feeling we don't, and it ends up depending on the internal 
implementation specifics of the regex compiler. It might work in this 
case, but I would be a little hesitant to depend on it.

For example if we have (.*\.ext)|(cfoo.*) Will 'cfoo.ext' always match 
the first pattern? (My quick tests with python2.5.2 says that it does, 
but that doesn't mean it would with all combinations of pre versus post, 
or using different regex libraries.)

What I might prefer is to have the OrderedGlobster just check *for* a 
match, and then iterate to ensure the right order. Then again, maybe I'm 
just being paranoid. (It would be nice if we could know *all* patterns 
that might match, rather than just which one did match.)

I did see that you added a test for this, and I suppose as long as it 
works with all active versions of python, we can use it. So if it is 
tested with python 2.4, 2.5, and 2.6, we can probably go with it for 
now.


I don't really prefer putting the file in as a versioned file in the 
users workspace, but I understand the tradeoffs and I'm will to concede 
on this one. I would just make a strong request for a required format 
marker at the top of the file. Having one for .bzrignore would have 
meant we could have retained proper compatibility across versions.


+``BZR_HOME/rules``. Either or both files may be missing. If a rule is

^- I *think* convention is to use $BZR_HOME/rules. Whatever our standard 
is, should be used.

I kind of feel like "patterns" should be its own topic, so that both 
ignore and rules can share it.

I don't see how "Preferences" fits in. It should either be earlier, or 
just gone entirely, since you describe them at the start. At a minimum 
it looks more like you are describing the INI syntax more than 
"Preferences". Linking out to the ConfigObj page seems more confusing 
than helpful, as what you really want is just the syntax of what is 
going to be allowed as rules entries.


+    def get_items(self, path, names=None):
+        """Return the preferences for a path as a sequence of 
name,value tuples.
+
+        :param path: tree relative path
+        :param names: the list of preferences to lookup - None for all
+        :return: None if no rule matched, otherwise a sequence of 
name,value
+          tuples. If names is not None, the sequence is the same length 
as
+          names, tuple order matches the order in names, and undefined
+          preferences are given the value None.
+        """
+        raise NotImplementedError(self.get_items)

^- The api here seems heavily dependent on whether names is supplied or 
not. (If you supply it, you always get return values, if you don't you 
sometimes get a sequence, sometimes get None.)

I don't really like apis that change their return values based on input 
arguments. I would recommend at a minimum returning an empty sequence if 
names is not supplied.

It also seems to be contradicted by this call:
+    def test_get_items_file_missing(self):
+        rs = self.make_searcher(None)
+        self.assertEquals(None, rs.get_items('a.txt'))
+        self.assertEquals(None, rs.get_items('a.txt', ['foo']))

^- Is this supposed to return None or [('foo', None)]. Maybe it needs to 
be clearer if there is nothing to match it returns None.


...
+    def make_searcher(self, lines):
+        """Make a _RulesSearcher from a list of strings"""
+        return rules._IniBasedRulesSearcher(lines)
+

...

+    def test_get_items_from_extension_match(self):
+        rs = self.make_searcher(["[*.txt]", "foo=bar", "a=True"])

^- Shouldn't these lines have '\n' at the end of them?
Even better, have it take "text" and then do the splitting yourself 
before handing it to _IniBasedRulesSearcher. I think it would be a lot 
easier for humans to understand than trying to follow the nested [] and 
quote characters.

I also wonder if we want '.bzrrules' to have an explicit file id. Like 
('bzr:rules', etc). Something to think about.


For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C483D087B.2030400%40internode.on.net%3E