Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cli): scope config interpolation to radon section #252

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kieran-ryan
Copy link

@kieran-ryan kieran-ryan commented Sep 2, 2023

Description

radon reads and passes the entire contents of a configuration file to a parser, rather than just its section. This parser supports interpolation (or automatic substitution) for values using the '%' sign and will raise an error if they are not escaped (using '%%') or do not match the required pattern (%\(([^)]+)\)s) - see #244.

While this is unlikely to cause an issue with standalone radon configuration files such as radon.cfg, with PEP 518 defining a common location in pyproject.toml for storing tool configuration in a [tool] table; users that install and run radon 6.0.0 onwards (with pyproject.toml support) will experience the aforementioned error if they are using '%' in any values in configuration sections that are unrelated to radon.

Thus, the issue can be resolved by either removing interpolation functionality for radon configuration completely OR by performing it against the radon section only. This PR applies the latter, with the removal of interpolation as something that could be applied in a future radon major version. It thus resolves #244 and resolves #251.

Detail

radon uses configparser.ConfigParser for parsing configuration files. This class performs interpolation using the '%' sign and they must be escaped where not required for interpolation with '%%'. For example, %(base_dir)s/tests would be substituted with the value of a base_dir key in the same section, however the raw not-interpolated value could be attained by adding another '%' at its beginning. See the Python documentation for further detail. radon also passes the entire configuration file to the parser class - not just its section.

radon 6.0.0 introduced pyproject.toml support and reads that file when using Python 3.11 (native toml support) - or earlier versions with tomli installed. Thus any values in pyproject.toml files that contain '%' signs and that do not match the interpolation pattern, will result in a ValueError for invalid interpolation syntax. This also holds true if such values are found in .ini or .cfg radon configuration files. However with PEP 518 defining a [tool] table for configuration of multiple tools and many projects moving more configuration into pyproject.toml, there may be increased likelihood of this occurring, compared to previously.

Solutions

There are different solutions available to eliminate this error for users.

Remove interpolation support

Removing interpolation support would prevent this error. This can be achieved by migrating the parser to configparser.RawConfigParser which does not perform substitutions.

- config = configparser.ConfigParser()
+ config = configparser.RawConfigParser()

It could be argued that there are few or no elements of radon configuration that users could use with interpolation today; however as usage is challenging to ascertain, reserving deprecation for a future major version may be the safest option.

Other Python static analysis tooling such as pytest and black do not appear to use interpolation.

Limit interpolation to radon section only

The default interpolator used by configparser.ConfigParser interpolates only within each section - though support across sections can be achieved by using configparser.ExtendedInterpolation instead. Therefore the parser does not need to be aware of other sections and we can pass the radon configuration to it only - preventing errors caused by unrelated configuration.

This can be achieved by using the raw configuration parser (configparser.RawConfigParser) mentioned in the previous section initially; and to then pass the radon section only to the current parser (configparser.ConfigParser) - where it will be interpolated - preventing breaking changes.

    def file_config():
        '''Return any file configuration discovered'''
-       config = configparser.ConfigParser()
+       config = configparser.RawConfigParser()
        for path in (os.getenv('RADONCFG', None), 'radon.cfg'):
            if path is not None and os.path.exists(path):
                config.read_file(open(path))
        config.read_dict(FileConfig.toml_config())
        config.read(['setup.cfg', os.path.expanduser('~/.radon.cfg')])

+       if config.has_section(CONFIG_SECTION_NAME):
+           section = dict(config.items(CONFIG_SECTION_NAME))
+           interpolated_config = configparser.ConfigParser()
+           interpolated_config.read_dict({CONFIG_SECTION_NAME: section})
+           config = interpolated_config

        return config

Removing the if clause in future would be the only change to deprecate interpolation - as mentioned in the previous section.

The read_dict method is not available in Python 2's (ConfigParser.ConfigParser) class and is thus incompatible. Importing the builtins in the radon.cli.harvest module is also not compatible with Python 2. If Python 2 support can be removed, a Python3-only deprecation warning could optionally be added as below to inform anyone using that feature of the future change. Though this PR focuses on addressing errors from unrelated configuration without deviating from the existing source code as much as possible; and deprecating Python 2 support may be more suitable as its own scoped issue due to the changes involved.

class BasicInterpolation(configparser.BasicInterpolation):

    _deprecation_warned = False

    def before_set(self, parser, section, option, value):
        tmp_value = value.replace('%%', '')
        if not self._deprecation_warned and self._KEYCRE.match(tmp_value):
            warnings.PendingDeprecationWarning(
                "interpolation support will be removed in a future radon major version"
            )
        return super().before_set(parser, section, option, value)

Mitigation

For users experiencing the issue with a pyproject.toml file, the issue can be mitigated by rolling back to 5.1.0 (if not dependent on versions later than 6.0.0) - as it does not have support to check that file - and by moving any radon configuration to an independent radon.cfg configuration file.

@kieran-ryan kieran-ryan marked this pull request as ready for review September 2, 2023 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Radon can't run when use pytest log fornat: $()d Radon crashes on unrelated config option in pyproject.toml
1 participant