This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/178/

On August 25th, 2015, 3:58 nachm. CEST, Nils Philippsen wrote:

src/rolekit/server/io/systemd.py (Diff revision 1)
257
        self.optionxform = lambda option: option

Is there a reason why this is not a normal (or class, or static) method? I.e.:

...
@staticmethod
def optionxform(option):
    "This method optionally transforms an option."
    return option
...

On August 25th, 2015, 9:51 nachm. CEST, Stephen Gallagher wrote:

No, it was just saving some typing, but it probably needed a comment. All it really needs to do is become a no-op. If you want me to make it a staticmethod, I will.

I think it's easier adding a doc comment (so it'll show up in pydoc ... or help(...)) if it's a method (no matter what type). I don't have a preference as to the method type, I didn't use the others because it doesn't need access to the class or object.


On August 25th, 2015, 3:58 nachm. CEST, Nils Philippsen wrote:

src/rolekit/server/io/systemd.py (Diff revision 1)
358
                            optval = optval.strip()
359
                            # Check if this optname already exists
360
                            if (optname in cursect) and (cursect[optname] is not None):
361
                                # If it does, convert it to a tuple if it isn't already one
362
                                if not isinstance(cursect[optname], tuple):
363
                                    cursect[optname] = tuple(cursect[optname])
364
                                cursect[optname] = cursect[optname] + tuple([optval])
365
                            else:
366
                                cursect[optname] = [optval]

Call this more of a gut-feeling, but I'm not sure I like the value of a section being a tuple only if it exists multiple times. I don't think this makes it easier on the user of the class, they need to check if the section exists more than once and then handle it differently than if it doesn't. Or do I misunderstand something?

On August 25th, 2015, 9:51 nachm. CEST, Stephen Gallagher wrote:

First, this is actually an option inside a section; it's not handling identically-named sections.

So, the first time an option name appears in the file, it's value is added directly. The second time the parser encounters that entry, it becomes a tuple. Anyone that consumes this configparser would need to detect whether the value is a tuple or not and handle it differently. The alternative would be to simply always have this turn into a tuple (rather than only on subsequent additions), but this would be a significant change from existing configparser behavior. I don't have a great answer for it. In our rolekit use-case, we are basically "write-only". The values are handled by the .write() function and get put into the destination unit files properly.

If you have a strong opinion one way or another, please voice it.

No, I don't have a strong opinion on it (I haven't used ConfigParser that often), it just rubbed me the wrong way. I agree that always using lists or tuples is ugly in its own way ;).


- Nils


On August 5th, 2015, 12:28 vorm. CEST, Stephen Gallagher wrote:

Review request for RoleKit Mailing List, Miloslav Trmac, Nils Philippsen, Stephen Gallagher, and Thomas Woerner.
By Stephen Gallagher.

Updated Aug. 5, 2015, 12:28 vorm.

Repository: rolekit

Description

This is a temporary measure; this code will be going upstream into python-systemd in the near future, but for now we will carry it in rolekit.

There is one known limitation to this implementation; It does not support reading unit files with multiline values. This won't be an issue in rolekit, but it's preventing it from going upstream to python-systemd at the moment.

Testing

Testing performed manually by loading the python module and writing out some sample unit files. Also tested with the kickstart patch.

Diffs

  • src/rolekit/server/io/systemd.py (3b6bca678f70a608e2d54bb3615523dec7ab9271)

View Diff