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: optionIs 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 ...
orhelp(...)
) 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
Testing
Diffs
|