Paul Price added a comment - 06/Apr/16 9:36 AM A real easy review, please Russell Owen : pprice@tiger-sumire:/tigress/pprice/dm-5124/pex_config (tickets/DM-5729=) $ git sub-patch commit c8eb449e2a30d86a236151899ea0463239fb826d Author: Paul Price <price@astro.princeton.edu> Date: Wed Apr 6 12:32:02 2016 -0400   allow NameError in loading to propagate NameError was being unintentionally suppressed (we want to suppress it to catch the root->config change, but there are other cases where it needs to propagate or the user will be blissfully ignorant of errors). Added a test that demonstrates the problem.   diff --git a/python/lsst/pex/config/config.py b/python/lsst/pex/config/config.py index 8960825..9add5d9 100644 --- a/python/lsst/pex/config/config.py +++ b/python/lsst/pex/config/config.py @@ -562,6 +562,8 @@ class Config(object): " appears to use 'root' instead of 'config'; trying with 'root'") local = {"root": self} exec stream in {}, local + else: + raise self._imports.update(importer.getModules()) diff --git a/tests/config.py b/tests/config.py index 5182e3b..99661aa 100755 --- a/tests/config.py +++ b/tests/config.py @@ -391,6 +391,12 @@ except ImportError: self.assert_("Inequality in r['AAA']" in output) self.assert_("Inequality in r['BBB']" not in output) + def testLoadError(self): + """Check that loading allows errors in the file being loaded to propagate + """ + self.assertRaises(SyntaxError, self.simple.loadFromStream, "bork bork bork") + self.assertRaises(NameError, self.simple.loadFromStream, "config.f = bork") + def suite(): utilsTests.init() suites = []

Paul Price added a comment - 06/Apr/16 9:38 AM I'm running Jenkins to see if any bad config changes have crept in that will now fail.

Paul Price added a comment - 06/Apr/16 11:00 AM Jenkins run passed. Of course, we've got no idea how many config override were silently failing before and are now producing different configs, but it's better than it having failed!

Russell Owen added a comment - 06/Apr/16 2:14 PM Thank you for catching and fixing this problem. Your fix will certainly work and it is simple. However, please consider removing the "root" support, instead. It was only ever intended to be temporary and removing it simplifies the code.

Paul Price added a comment - 06/Apr/16 5:29 PM If you don't mind, I will leave the backward-compatibility support for root in place for now. Removing it would require much wider consultation, and I don't think it's a good idea right now because we're about to move HSC people to the LSST stack and we will want to present them a nice warning rather than a perplexing error message. But please don't let me stop you from RFC-ing your proposal and adding a ticket for the work to be scheduled for the future.

Paul Price added a comment - 07/Apr/16 1:31 PM Merged to master after Russell mentioned on HipChat that he agrees.

John Swinbank added a comment - 11/Apr/16 6:01 AM Mentioned in release notes.