This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/219/ |
config/roles/databaseserver/role.py (Diff revision 2) | |||
---|---|---|---|
51 | def tweak_lines(lines_iterable, tweaking_rules, append_if_missing=False): |
||
52 | for rule in tweaking_rules: |
||
53 | regex = rule['regex'] |
||
54 | if isinstance(regex, str): |
||
55 | rule['regex'] = re.compile(regex) |
||
56 | rule.setdefault('global', True) |
||
57 | rule.setdefault('append_if_missing', append_if_missing) |
||
58 | |||
59 | found_regexes = set() |
||
60 | ignore_regexes = set() |
||
61 | |||
62 | for line in lines_iterable: |
||
63 | lines_to_append = [] |
||
64 | for rule in tweaking_rules: |
||
65 | regex = rule['regex'] |
||
66 | if regex not in ignore_regexes: |
||
67 | m = regex.search(line) |
||
68 | if m: |
||
69 | found_regexes.add(regex) |
||
70 | if not rule['global']: |
||
71 | ignore_regexes.add(regex) |
||
72 | if 'replace' in rule: |
||
73 | line = regex.sub(rule['replace'], line) |
||
74 | if 'append' in rule: |
||
75 | lines_to_append.append(rule['append']) |
||
76 | yield line |
||
77 | for l in lines_to_append: |
||
78 | yield l + "\n" |
||
79 | |||
80 | for rule in tweaking_rules: |
||
81 | if ( |
||
82 | rule['append_if_missing'] and |
||
83 | rule['regex'] not in found_regexes): |
||
84 | if 'replace' in rule: |
||
85 | yield rule['replace'] + "\n" |
||
86 | if 'append' in rule: |
||
87 | yield rule['append'] + "\n" |
||
88 | |||
89 |
This function is very difficult to follow. Please add many comments.
config/roles/databaseserver/role.py (Diff revision 2) | |||
---|---|---|---|
66 | if regex not in ignore_regexes: |
||
67 | m = regex.search(line) |
||
68 | if m: |
||
69 | found_regexes.add(regex) |
||
70 | if not rule['global']: |
||
71 | ignore_regexes.add(regex) |
How are we defining "global" regexes here? Per-file or per-line? That needs explanation. Right now, it looks like it's per-file.
config/roles/databaseserver/role.py (Diff revision 2) | |||
---|---|---|---|
81 | if ( |
||
82 | rule['append_if_missing'] and |
||
83 | rule['regex'] not in found_regexes): |
This whitespace usage is uncommon and hard to read.
config/roles/databaseserver/role.py (Diff revision 2) | |||
---|---|---|---|
305 | linkfile(conffile, bakfile) |
Can you add a comment about linkfile() here? Is this creating a hard-link? (And if so, is it also going to be overwritten by overwrite_safely()?)
- Stephen Gallagher
On September 19th, 2015, 1:04 a.m. UTC, Nils Philippsen wrote:
Review request for RoleKit Mailing List, Miloslav Trmac, Nils Philippsen, Stephen Gallagher, and Thomas Woerner.
By Nils Philippsen.
Updated Sept. 19, 2015, 1:04 a.m.
Repository:
rolekit
Description
Testing
Diffs
|