On Aug. 25, 2015, 3:58 nachm., Nils Philippsen wrote:
> src/rolekit/server/io/systemd.py, line 257
>
<
http://reviewboard-fedoraserver.rhcloud.com/r/178/diff/1/?file=791#file79...
>
> 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
> ...
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 Aug. 25, 2015, 3:58 nachm., Nils Philippsen wrote:
> src/rolekit/server/io/systemd.py, lines 358-366
>
<
http://reviewboard-fedoraserver.rhcloud.com/r/178/diff/1/?file=791#file79...
>
> 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?
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
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-fedoraserver.rhcloud.com/r/178/#review475
-----------------------------------------------------------
On Aug. 5, 2015, 12:28 vorm., Stephen Gallagher wrote:
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-fedoraserver.rhcloud.com/r/178/
-----------------------------------------------------------
(Updated Aug. 5, 2015, 12:28 vorm.)
Review request for RoleKit Mailing List, Miloslav Trmac, Nils Philippsen, Stephen
Gallagher, and Thomas Woerner.
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.
Diffs
-----
src/rolekit/server/io/systemd.py 3b6bca678f70a608e2d54bb3615523dec7ab9271
Diff:
http://reviewboard-fedoraserver.rhcloud.com/r/178/diff/
Testing
-------
Testing performed manually by loading the python module and writing out some sample unit
files. Also tested with the kickstart patch.
Thanks,
Stephen Gallagher