-
Notifications
You must be signed in to change notification settings - Fork 223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor the configure_workers_and_start.py
script used internally by Complement.
#16803
Changes from 1 commit
b39a50a
a22eb7d
ba3b6a4
67d4fc8
94a85b3
26073fa
8b74639
f38297b
7d8824e
f49dbc7
3bb21a9
fbafde8
321d359
259a808
3a46cf0
2f1d727
ad4bb0e
2ff1de3
29541fd
c91ab4b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -283,6 +283,31 @@ def flush_buffers() -> None: | |
sys.stderr.flush() | ||
|
||
|
||
def merge_into(dest: Any, new: Any) -> None: | ||
""" | ||
Merges `new` into `dest` with the following rules: | ||
|
||
- dicts: values with the same key will be merged recursively | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general I'm very cautious about this, as deep merging can be quite a foot gun. Sometimes it makes sense to merge configuration dicts, sometimes it really doesn't and can be very surprising. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you have any examples of where it doesn't make sense? I can't remember ever seeing one (and I use NixOS which is all about merging config dicts...!), but on the other hand I have been bitten by dicts not being merged quite a few times :). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The common case would be e.g. database config, where you'd have some config for a sqlite db and then it gets merged into config for a postgres db, and now you have a config with mixed parameters from both. I believe nixos gets around this by erroring out if the same key is set to different values in both dicts There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean yes I guess, but I don't consider that merging if two different values have been specified for the same key (it will raise an exception for this case, see https://github.com/element-hq/synapse/pull/16803/files#diff-30887e6a3a63f861b737500ca8243d3da5f0e7df016cc9cc020b8eeb61c2320cR355-R356). |
||
- lists: `new` will be appended to `dest` | ||
- primitives: they will be checked for equality and inequality will result | ||
in a ValueError | ||
|
||
It is an error for `dest` and `new` to be of different types. | ||
""" | ||
if isinstance(dest, dict) and isinstance(new, dict): | ||
for k, v in new.items(): | ||
if k in dest: | ||
merge_into(dest[k], v) | ||
else: | ||
dest[k] = v | ||
elif isinstance(dest, list) and isinstance(new, list): | ||
dest.extend(new) | ||
elif type(dest) != type(new): | ||
raise TypeError(f"Cannot merge {type(dest).__name__} and {type(new).__name__}") | ||
elif dest != new: | ||
raise ValueError(f"Cannot merge primitive values: {dest!r} != {new!r}") | ||
|
||
|
||
def convert(src: str, dst: str, **template_vars: object) -> None: | ||
"""Generate a file from a template | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name
into
makes me expect the second param is what is being changed, i.edest
intonew
. Maybemerge_update
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tempted just to replace its use with
merged
and havemerged
leave the input untouched