bpo-41086: Add exception for uninstantiated interpolation (configparser) #21062

scrummyin · 2020-06-23T01:29:40Z

The current feedback when users try to pass an uninstantiated
interpolation into a ConfigParser is an error message that does not help
users solve the problem. This current error of TypeError: before_set() missing 1 required positional argument: 'value' does not display until
the parser is used, which usually results in the assumption that
instantiation of the parser was done correctly. The new exception of
InterpolationIsNotInstantiatedError, will be raised on the line where
the ConfigParser is instantiated. This will result in users see the line
that has the error in their backtrace for faster debugging.

There have been a number of bugs created in the issue tracker, which
could have been addressed by:
https://bugs.python.org/issue26831 and https://bugs.python.org/issue26469

https://bugs.python.org/issue41086

the-knights-who-say-ni · 2020-06-23T01:29:44Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@scrummyin

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

scrummyin · 2020-06-23T16:34:41Z

Odd, that the Ubuntu check is failing, because this was built and tested on Ubuntu. So it works on my local and it looks like the errors are unrelated to work that I did. Hopefully one of the reviews can help.

iritkatriel · 2021-10-22T08:51:33Z

Lib/configparser.py

@@ -633,6 +642,8 @@ def __init__(self, defaults=None, dict_type=_default_dict,
            self._interpolation = self._DEFAULT_INTERPOLATION
        if self._interpolation is None:
            self._interpolation = Interpolation()
+        if isinstance(self._interpolation, type):
+            raise InterpolationIsNotInstantiatedError()


If the parameter is to be checked, then I think the check should be that interpolation is an instance of Interpolation (rather than that is is not an instance of type). That way you would catch other bad args.

The exception would be InterpolationTypeError or something like that.

Thanks for the suggestions, I've accepted them locally, but they cause some need for changes in the English text/explanations associated with the code. I'll made updates to those and post back once I get better text in there.

In response to the feedback, the exception now is InterpolationTypeError and some of the comment text and the commit message has been updated to reflect this. The patch was also rebased onto main as there was a comment on https://bugs.python.org/issue41086 about not backporting and a conflict in the __all__ which the rebase has resolved.

@iritkatriel , sorry to send a notification, but I wanted to check if I corrected things per your suggestion?

The current feedback when users try to pass an uninstantiated interpolation into a ConfigParser is an error message that does not help users solve the problem. This current error of `TypeError: before_set() missing 1 required positional argument: 'value'` does not display until the parser is used, which usually results in the assumption that instantiation of the parser was done correctly. The new exception of InterpolationTypeError, will be raised on the line where the ConfigParser is instantiated. This will result in users see the line that has the error in their backtrace for faster debugging. There have been a number of bugs created in the issue tracker, which could have been addressed by: https://bugs.python.org/issue26831 and https://bugs.python.org/issue26469

iritkatriel · 2022-01-27T16:11:29Z

Lib/configparser.py

+
+    Giving users faster and more clear feedback on how to fix a common error
+    """
+    def __init__(self):


The other exceptions in the module call Error.init, this one should follow this pattern.

iritkatriel · 2022-01-27T16:12:29Z

Lib/configparser.py

+class InterpolationTypeError(Error):
+    """Raised when the _interpolation is not an instance of Interpolation
+
+    Giving users faster and more clear feedback on how to fix a common error


The last line of this docstring isn't really providing useful information.

iritkatriel · 2022-01-27T16:15:33Z

Lib/test/test_configparser.py

@@ -1048,6 +1048,12 @@ def test_set_malformatted_interpolation(self):
        self.assertEqual(cf.get("sect", "option2"), "foo%%bar")


+class ConfigParserTestCaseInterpolationIsInstantiated(unittest.TestCase):


I'd make this ConfigParserTestCaseInvalidInterpolationType and add a couple of tests for other invalid types (say, an int and a string. You can do this in a loop), something like:

for value in [configparser.ExtendedInterpolation, 42, "a string]: with self.subTest(value = value): with self.assertRaises(TypeError): configparser.ConfigParser(interpolation=value)

iritkatriel · 2022-01-27T16:18:38Z

Lib/configparser.py

@@ -279,6 +279,13 @@ class InterpolationSyntaxError(InterpolationError):
    which substitutions are made does not conform to the required syntax.
    """

+class InterpolationTypeError(Error):


If you add a new exception type then you need to add it to the documentation as well. TBH, I don't think you need a new exception type here. You could just raise a TypeError.

iritkatriel · 2022-01-27T16:26:04Z

Lib/test/test_configparser.py

@@ -1048,6 +1048,12 @@ def test_set_malformatted_interpolation(self):
        self.assertEqual(cf.get("sect", "option2"), "foo%%bar")


+class ConfigParserTestCaseInterpolationIsInstantiated(unittest.TestCase):
+    def test_error_on_uninstantiated_interpolation(self):
+        with self.assertRaises(configparser.InterpolationTypeError) as context:


Suggested change

with self.assertRaises(configparser.InterpolationTypeError) as context:

with self.assertRaises(configparser.InterpolationTypeError):

iritkatriel · 2022-01-27T16:26:14Z

Lib/test/test_configparser.py

+class ConfigParserTestCaseInterpolationIsInstantiated(unittest.TestCase):
+    def test_error_on_uninstantiated_interpolation(self):
+        with self.assertRaises(configparser.InterpolationTypeError) as context:
+            cf = configparser.ConfigParser(interpolation=configparser.ExtendedInterpolation)


Suggested change

cf = configparser.ConfigParser(interpolation=configparser.ExtendedInterpolation)

configparser.ConfigParser(interpolation=configparser.ExtendedInterpolation)

iritkatriel · 2022-01-27T16:28:05Z

Misc/NEWS.d/next/Library/2020-06-23-01-50-24.bpo-41086.YnOvpS.rst

@@ -0,0 +1 @@
+Added validation check of instantiation on ConfigParser's interpolation parameter.


Suggested change

Added validation check of instantiation on ConfigParser's interpolation parameter.

Make the :class:`configparser.ConfigParser` constructor raise :exc:`TypeError` if the ``interpolation`` parameter is not of type :class:`configparser.Interpolation`.

iritkatriel · 2022-01-27T16:38:06Z

@scrummyin Thank you for the patch. I reviewed it and made some comments. However, I don't know whether the documentation would need changing. I can see in the docstring that the interpolation parameter is expected to be a subtype of Intrerpolation, but this not mentioned in the doc, so I don't know if it's ok to just block duck typing here.

@ambv - since you wrote the docstring, any thoughts?

@iritkatriel

Per feedback from @iritkatriel, the custom InterpolationTypeError has been dropped in favour of a TypeError with a custom message, and the unittests have been expanded.

scrummyin · 2022-01-28T15:31:42Z

@iritkatriel , I resolved most of your suggestions by removing the custom Error and switching it to TypeError. In addition, I also took the suggestions of adding more test coverage and the better news message. I've added a new commit for that work, and would be happy to squash the commits anyway you see fit. For now I left it as a new commit, in hopes that will facilitate future reviews.

Lib/configparser.py

ambv · 2022-02-17T12:17:54Z

Thanks! ✨ 🍰 ✨

the-knights-who-say-ni added the CLA not signed label Jun 23, 2020

bedevere-bot added the awaiting review label Jun 23, 2020

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Jun 23, 2020

iritkatriel reviewed Oct 22, 2021

View reviewed changes

scrummyin force-pushed the fix/configparser_interpolation branch from b4808ee to f593811 Compare October 22, 2021 12:39

scrummyin and others added 2 commits October 23, 2021 21:00

📜🤖 Added by blurb_it.

5548184

scrummyin force-pushed the fix/configparser_interpolation branch from f593811 to 5548184 Compare October 24, 2021 01:01

iritkatriel reviewed Jan 27, 2022

View reviewed changes

Replace custom Error with TypeError

5e790cb

Per feedback from @iritkatriel, the custom InterpolationTypeError has been dropped in favour of a TypeError with a custom message, and the unittests have been expanded.

ambv reviewed Feb 17, 2022

View reviewed changes

Lib/configparser.py Outdated Show resolved Hide resolved

More verbose message

0c1c7a6

ambv merged commit fc115c9 into python:main Feb 17, 2022

bedevere-bot removed the awaiting review label Feb 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-41086: Add exception for uninstantiated interpolation (configparser) #21062

bpo-41086: Add exception for uninstantiated interpolation (configparser) #21062

scrummyin commented Jun 23, 2020 •

edited by bedevere-bot

Loading

the-knights-who-say-ni commented Jun 23, 2020

scrummyin commented Jun 23, 2020 •

edited

Loading

iritkatriel Oct 22, 2021

scrummyin Oct 22, 2021

scrummyin Oct 24, 2021

scrummyin Jan 27, 2022

iritkatriel Jan 27, 2022

iritkatriel Jan 27, 2022

iritkatriel Jan 27, 2022

iritkatriel Jan 27, 2022

iritkatriel Jan 27, 2022

iritkatriel Jan 27, 2022

iritkatriel Jan 27, 2022

iritkatriel commented Jan 27, 2022

scrummyin commented Jan 28, 2022

ambv commented Feb 17, 2022

		@@ -1048,6 +1048,12 @@ def test_set_malformatted_interpolation(self):
		self.assertEqual(cf.get("sect", "option2"), "foo%%bar")


		class ConfigParserTestCaseInterpolationIsInstantiated(unittest.TestCase):

	with self.assertRaises(configparser.InterpolationTypeError) as context:
	with self.assertRaises(configparser.InterpolationTypeError):

	cf = configparser.ConfigParser(interpolation=configparser.ExtendedInterpolation)
	configparser.ConfigParser(interpolation=configparser.ExtendedInterpolation)

		@@ -0,0 +1 @@
		Added validation check of instantiation on ConfigParser's interpolation parameter.

	Added validation check of instantiation on ConfigParser's interpolation parameter.
	Make the :class:`configparser.ConfigParser` constructor raise :exc:`TypeError` if the ``interpolation`` parameter is not of type :class:`configparser.Interpolation`.

bpo-41086: Add exception for uninstantiated interpolation (configparser) #21062

bpo-41086: Add exception for uninstantiated interpolation (configparser) #21062

Conversation

scrummyin commented Jun 23, 2020 • edited by bedevere-bot Loading

the-knights-who-say-ni commented Jun 23, 2020

Recognized GitHub username

scrummyin commented Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iritkatriel commented Jan 27, 2022

scrummyin commented Jan 28, 2022

ambv commented Feb 17, 2022

scrummyin commented Jun 23, 2020 •

edited by bedevere-bot

Loading

scrummyin commented Jun 23, 2020 •

edited

Loading