#1150 using ConfigParser.read_file for PY3
Merged 5 years ago by mikem. Opened 5 years ago by julian8628.
julian8628/koji issue/1075  into  master

file modified
+1 -4
@@ -5821,10 +5821,7 @@ 

          if not os.path.exists(task_options.config):

              parser.error(_("%s not found!" % task_options.config))

          section = 'image-build'

-         config = six.moves.configparser.ConfigParser()

-         conf_fd = open(task_options.config)

-         config.readfp(conf_fd)

-         conf_fd.close()

+         config = koji.read_config_files(task_options.config)

          if not config.has_section(section):

              parser.error(_("single section called [%s] is required" % section))

          # pluck out the positional arguments first

file modified
+30 -4
@@ -1698,10 +1698,7 @@ 

      # Load the configs in a particular order

      got_conf = False

      for configFile in configs:

-         f = open(configFile)

-         config = six.moves.configparser.ConfigParser()

-         config.readfp(f)

-         f.close()

+         config = read_config_files(configFile)

          if config.has_section(profile_name):

              got_conf = True

              for name, value in config.items(profile_name):
@@ -1795,6 +1792,35 @@ 

      return mod

  

  

+ def read_config_files(config_files, raw=False):

+     """Use parser to read config file(s)

+ 

+     :param config_files: config file(s) to read (required).

+     :type config_files: str or list

+     :param bool raw: enable 'raw' parsing (no interpolation). Default: False

+ 

+     :return: object of parser which contains parsed content

+     """

+     if not isinstance(config_files, (list, tuple)):

+         config_files = [config_files]

+     if raw:

+         parser = six.moves.configparser.RawConfigParser

+     elif six.PY2:

+         parser = six.moves.configparser.SafeConfigParser

+     else:

+         # In python3, ConfigParser is "safe", and SafeConfigParser is a

+         # deprecated alias

+         parser = six.moves.configparser.ConfigParser

+     config = parser()

+     for config_file in config_files:

+         with open(config_file, 'r') as f:

+             if six.PY2:

+                 config.readfp(f)

+             else:

+                 config.read_file(f)

+     return config

+ 

+ 

  class PathInfo(object):

      # ASCII numbers and upper- and lower-case letter for use in tmpdir()

      ASCII_CHARS = [chr(i) for i in list(range(48, 58)) + list(range(65, 91)) + list(range(97, 123))]

file modified
+4 -9
@@ -36,7 +36,6 @@ 

  import struct

  import sys

  import time

- import six.moves.configparser

  from zlib import adler32

  from six.moves import range

  import six
@@ -733,13 +732,7 @@ 

  

      Return a map whose keys are package names and values are config parameters.

      """

-     if not isinstance(confs, (list, tuple)):

-         confs = [confs]

-     config = six.moves.configparser.ConfigParser()

-     for conf in confs:

-         conf_fd = open(conf)

-         config.readfp(conf_fd)

-         conf_fd.close()

+     config = koji.read_config_files(confs)

      builds = {}

      for package in config.sections():

          buildtype = 'maven'
@@ -753,10 +746,12 @@ 

                  raise ValueError("A wrapper-rpm must depend on exactly one package")

          else:

              raise ValueError("Unsupported build type: %s" % buildtype)

-         if not 'scmurl' in params:

+         if 'scmurl' not in params:

              raise ValueError("%s is missing the scmurl parameter" % package)

          builds[package] = params

      if not builds:

+         if not isinstance(confs, (list, tuple)):

+             confs = [confs]

          raise ValueError("No sections found in: %s" % ', '.join(confs))

      return builds

  

file modified
+1 -5
@@ -9,7 +9,6 @@ 

  import koji

  from koji.plugin import callback, ignore_error, convert_datetime

  from koji.context import context

- import six.moves.configparser

  import logging

  import json

  import random
@@ -270,10 +269,7 @@ 

      log = logging.getLogger('koji.plugin.protonmsg')

      global CONFIG

      if not CONFIG:

-         conf = six.moves.configparser.SafeConfigParser()

-         with open(CONFIG_FILE) as conffile:

-             conf.readfp(conffile)

-         CONFIG = conf

+         CONFIG = koji.read_config_files(CONFIG_FILE)

      urls = CONFIG.get('broker', 'urls').split()

      test_mode = False

      if CONFIG.has_option('broker', 'test_mode'):

file modified
+54 -2
@@ -158,6 +158,54 @@ 

              m.assert_not_called()

  

  

+ class ConfigFileTestCase(unittest.TestCase):

+     """Test config file reading functions"""

+ 

+     @mock_open()

+     @mock.patch("six.moves.configparser.ConfigParser", spec=True)

+     @mock.patch("six.moves.configparser.RawConfigParser", spec=True)

+     @mock.patch("six.moves.configparser.SafeConfigParser", spec=True)

+     def test_read_config_files(self, scp_clz, rcp_clz, cp_clz, open_mock):

+         files = 'test1.conf'

+         conf = koji.read_config_files(files)

+         open_mock.assert_called_once_with(files, 'r')

+         if six.PY2:

+             self.assertTrue(isinstance(conf,

+                                        six.moves.configparser.SafeConfigParser.__class__))

+             scp_clz.assert_called_once()

+             scp_clz.return_value.readfp.assert_called_once()

+         else:

+             self.assertTrue(isinstance(conf,

+                                        six.moves.configparser.ConfigParser.__class__))

+             cp_clz.assert_called_once()

+             cp_clz.return_value.read_file.assert_called_once()

+ 

+         open_mock.reset_mock()

+         cp_clz.reset_mock()

+         scp_clz.reset_mock()

+         files = ['test1.conf', 'test2.conf']

+         koji.read_config_files(files)

+         open_mock.assert_has_calls([call('test1.conf', 'r'),

+                                     call('test2.conf', 'r')],

+                                    any_order=True)

+         if six.PY2:

+             scp_clz.assert_called_once()

+             self.assertEqual(scp_clz.return_value.readfp.call_count, 2)

+         else:

+             cp_clz.assert_called_once()

+             self.assertEqual(cp_clz.return_value.read_file.call_count, 2)

+ 

+         open_mock.reset_mock()

+         cp_clz.reset_mock()

+         scp_clz.reset_mock()

+         conf = koji.read_config_files(files, raw=True)

+         self.assertTrue(isinstance(conf,

+                                    six.moves.configparser.RawConfigParser.__class__))

+         cp_clz.assert_not_called()

+         scp_clz.assert_not_called()

+         rcp_clz.assert_called_once()

+ 

+ 

  class MavenUtilTestCase(unittest.TestCase):

      """Test maven relative functions"""

      maxDiff = None
@@ -491,10 +539,14 @@ 

          self.assertEqual(cm.exception.args[0], 'total ordering not possible')

  

      def _read_conf(self, cfile):

-         config = six.moves.configparser.ConfigParser()

          path = os.path.dirname(__file__)

          with open(path + cfile, 'r') as conf_file:

-             config.readfp(conf_file)

+             if six.PY2:

+                 config = six.moves.configparser.SafeConfigParser()

+                 config.readfp(conf_file)

+             else:

+                 config = six.moves.configparser.ConfigParser()

+                 config.read_file(conf_file)

          return config

  

      def test_formatChangelog(self):

@@ -9,7 +9,7 @@ 

  

  from mock import patch, MagicMock

  from koji.context import context

- from six.moves.configparser import SafeConfigParser

+ from six.moves.configparser import ConfigParser, SafeConfigParser

  

  class TestProtonMsg(unittest.TestCase):

      def tearDown(self):
@@ -269,8 +269,12 @@ 

  connect_timeout = 10

  send_timeout = 60

  """)

-         conf = SafeConfigParser()

-         conf.readfp(confdata)

+         if six.PY2:

+             conf = SafeConfigParser()

+             conf.readfp(confdata)

+         else:

+             conf = ConfigParser()

+             conf.read_file(confdata)

          self.handler = protonmsg.TimeoutHandler('amqps://broker1.example.com:5671', [], conf)

  

      @patch('protonmsg.SSLDomain')
@@ -290,8 +294,12 @@ 

  connect_timeout = 10

  send_timeout = 60

  """)

-         conf = SafeConfigParser()

-         conf.readfp(confdata)

+         if six.PY2:

+             conf = SafeConfigParser()

+             conf.readfp(confdata)

+         else:

+             conf = ConfigParser()

+             conf.read_file(confdata)

          handler = protonmsg.TimeoutHandler('amqp://broker1.example.com:5672', [], conf)

          event = MagicMock()

          handler.on_start(event)

Want to add a docstring here to indicate what you expect "parser" to be? "six.moves.configparser.ConfigParser or "six.moves.configparser.SafeConfigParser")

rebased onto b365fa7e4367a733742811acff5b6e424ca41af4

5 years ago

Want to add a docstring here to indicate what you expect "parser" to be? "six.moves.configparser.ConfigParser or "six.moves.configparser.SafeConfigParser")

updated

Want to add a docstring here to indicate what you expect "parser" to be?

Taking this a step further, it seems that the option is never used. So, it's worth asking if we even should provide it.

I'm guessing the thinking is that we may in the future want to use one of the other parsers provided by the ConfigParser module (e.g. RawConfigParser, SafeConfigParser). We use RawConfigParser in the hub and web code, for example.

While we're here, it may be worth defaulting to SafeConfigParser.

And given that, I almost wonder if instead of a parser option, we might want a raw option that toggles between SafeConfigParser and RawConfigParser.

@mikem I wonder if we could drop all readfp and read_files invokings, and use read instead.

And given that, I almost wonder if instead of a parser option, we might want a raw option that toggles between SafeConfigParser and RawConfigParser.

+1

FYI SafeConfigParser will no longer exist in a future Python release,

$ python3
Python 3.7.2 (default, Jan  3 2019, 09:14:01) 
[GCC 8.2.1 20181215 (Red Hat 8.2.1-6)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from configparser import SafeConfigParser
>>> SafeConfigParser()
__main__:1: DeprecationWarning: The SafeConfigParser class has been renamed to ConfigParser in Python 3.2. This alias will be removed in future versions. Use ConfigParser directly instead.

https://docs.python.org/3/whatsnew/3.2.html#configparser

@julian8628 You can also go through PR #866 and pick what is usable from there (as this one mostly obsoletes that PR)

That change looks good to me Mike

rebased onto a8d8352

5 years ago

@mikem thanks, that's good
I've updated this pr and made some minor fixes for invoking and testing

I'm going to check config.read() usage based on PR #866

Thanks for fixing the unit test.

I'm going to check config.read() usage based on PR #866

Do you mean to update this pr with more changes then? If so, please let me know. Otherwise I'll merge this tomorrow.

Thanks for fixing the unit test.

I'm going to check config.read() usage based on PR #866

Do you mean to update this pr with more changes then? If so, please let me know. Otherwise I'll merge this tomorrow.

I would file another PR for the rest of works, which should not a block for python3 stuff.
Feel free to merge this

Commit 8813d28 fixes this pull-request

Pull-Request has been merged by mikem

5 years ago