[MERGE] Conditionalize format warnings

Martin Pool mbp at sourcefrog.net
Thu Jul 12 09:36:24 BST 2007


On 7/4/07, Matthew D. Fuller <fullermd at over-yonder.net> wrote:
> On Mon, Jul 02, 2007 at 10:33:00PM +1000 I heard the voice of
> Ian Clatworthy, and lo! it spake thus:
> >
> > A quick browse of the list email suggests this is back with you now.
> > Is that correct?
>
> It is.  Hasn't gone anywhere, though; too many other things keeping it
> from shouting distance of the top of my stack.

It would be cool to have this in sometime.  I'm going to remove it
from bb just so we can focus on what still needs review.

I did look at it again and commented:

Hi,

@@ -272,6 +275,20 @@
     def _get_nickname(self):
         return None

+    def get_format_warning(self):
+        """Warn for outdated formats?"""
+        dowarn = self._get_format_warning()
+        if dowarn is not None and dowarn.lower() == "no":
+            return "no"
+        return "yes"
+
+    def _get_format_warning(self):
+        dowarn = self.get_user_option('format_warning')
+        if dowarn not in (None, "yes", "no"):
+            raise AssertionError("Unknown format_warning value '%s' in %s"
+                                 % (dowarn, self._get_filename()))
+        return dowarn
+

This seems like a lot of proliferation of code for what is really just a
boolean variable with a default.  I think I'd rather see, say,

	config.get_boolean('format_warning', default=True)

At least that should be the implementation of get_format_warning, since we
do have a method for most of the other configuration options.

@@ -832,6 +852,8 @@
     """Branch configuration data associated with its contents, not location"""
     def __init__(self, branch):
         self.branch = branch
+        self._get_filename = \
+            lambda: branch.control_files.controlfilename('branch.conf')

Why do we need to set _get_nickname here?  It seems like this object
already knows how to get this file from the TreeConfig.  (Which is a bit
strange, because it then goes to ask the branch for its configuration...)

Thanks for the tests.

I'd be happy to have this feature, but not quite this patch.

-- 
Martin



More information about the bazaar mailing list