Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add config_overrides argument to initializer of MergerConfig #225

Merged
merged 10 commits into from
Feb 10, 2024
Merged

Add config_overrides argument to initializer of MergerConfig #225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
13a3038
Pass args through config property of MergerConfig
leviem1 Oct 23, 2023
fe2bd4c
Use kwargs, store as property
leviem1 Oct 23, 2023
7ec2e51
Rename user_config to config_overrides
leviem1 Oct 28, 2023
741d6f5
Fix type issues
leviem1 Oct 29, 2023
cc5812a
Remove redundant defaults option
leviem1 Oct 29, 2023
db4e75b
Add tests
leviem1 Oct 29, 2023
8fc4344
Pass overrides directly to _load_config()
leviem1 Oct 31, 2023
be67e61
Use pop over index in ctor
leviem1 Nov 8, 2023
edcbb5b
Add tests for running without config file
leviem1 Nov 10, 2023
8501093
Remove requirement of config file
leviem1 Nov 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
288 changes: 288 additions & 0 deletions tests/test_merger_mergerconfig.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
create_temp_yaml_file
)


class Test_merger_MergerConfig():
"""Tests for the MergerConfig class."""

Expand Down Expand Up @@ -207,6 +208,83 @@ def test_hash_merge_mode_ini_rule_overrides_cli(
assert mc.hash_merge_mode(
NodeCoords(node, parent, parentref)) == mode

@pytest.mark.parametrize("ini_rule, override_rule, mode", [
("left", "right", HashMergeOpts.RIGHT),
("right", "deep", HashMergeOpts.DEEP),
("deep", "left", HashMergeOpts.LEFT),
])
def test_hash_merge_mode_override_rule_overrides_ini_rule(
self, quiet_logger, tmp_path_factory, ini_rule, override_rule, mode
):
config_file = create_temp_yaml_file(tmp_path_factory, """
[rules]
/hash = {}
""".format(ini_rule))
lhs_yaml_file = create_temp_yaml_file(tmp_path_factory, """---
hash:
lhs_exclusive: lhs value 1
merge_targets:
subkey: lhs value 2
subarray:
- one
- two
array_of_hashes:
- name: LHS Record 1
id: 1
prop: LHS value AoH 1
- name: LHS Record 2
id: 2
prop: LHS value AoH 2
""")
lhs_yaml = get_yaml_editor()
(lhs_data, lhs_loaded) = get_yaml_data(lhs_yaml, quiet_logger, lhs_yaml_file)

mc = MergerConfig(quiet_logger, SimpleNamespace(config=config_file), rules={"/hash": override_rule})
mc.prepare(lhs_data)

node = lhs_data["hash"]
parent = lhs_data
parentref = "hash"

assert mc.hash_merge_mode(
NodeCoords(node, parent, parentref)) == mode

@pytest.mark.parametrize("arg_rule, override_rule, mode", [
("left", "right", HashMergeOpts.RIGHT),
("right", "deep", HashMergeOpts.DEEP),
("deep", "left", HashMergeOpts.LEFT),
])
def test_hash_merge_mode_override_rule_overrides_arg_rule(
self, quiet_logger, tmp_path_factory, arg_rule, override_rule, mode
):
lhs_yaml_file = create_temp_yaml_file(tmp_path_factory, """---
hash:
lhs_exclusive: lhs value 1
merge_targets:
subkey: lhs value 2
subarray:
- one
- two
array_of_hashes:
- name: LHS Record 1
id: 1
prop: LHS value AoH 1
- name: LHS Record 2
id: 2
prop: LHS value AoH 2
""")
lhs_yaml = get_yaml_editor()
(lhs_data, lhs_loaded) = get_yaml_data(lhs_yaml, quiet_logger, lhs_yaml_file)

mc = MergerConfig(quiet_logger, SimpleNamespace(hashes=arg_rule), rules={"/hash": override_rule})
mc.prepare(lhs_data)

node = lhs_data["hash"]
parent = lhs_data
parentref = "hash"

assert mc.hash_merge_mode(
NodeCoords(node, parent, parentref)) == mode

###
# array_merge_mode
Expand Down Expand Up @@ -311,6 +389,93 @@ def test_array_merge_mode_ini_rule_overrides_cli(
assert mc.array_merge_mode(
NodeCoords(node, parent, parentref)) == mode

@pytest.mark.parametrize("ini_rule, override_rule, mode", [
("left", "right", ArrayMergeOpts.RIGHT),
("right", "unique", ArrayMergeOpts.UNIQUE),
("unique", "all", ArrayMergeOpts.ALL),
("all", "left", ArrayMergeOpts.LEFT),
])
def test_array_merge_mode_override_rule_overrides_ini_rule(
self, quiet_logger, tmp_path_factory, ini_rule, override_rule, mode
):
config_file = create_temp_yaml_file(tmp_path_factory, """
[rules]
/hash/merge_targets/subarray = {}
""".format(ini_rule))
lhs_yaml_file = create_temp_yaml_file(tmp_path_factory, """---
hash:
lhs_exclusive: lhs value 1
merge_targets:
subkey: lhs value 2
subarray:
- one
- two
array_of_hashes:
- name: LHS Record 1
id: 1
prop: LHS value AoH 1
- name: LHS Record 2
id: 2
prop: LHS value AoH 2
""")
lhs_yaml = get_yaml_editor()
(lhs_data, lhs_loaded) = get_yaml_data(lhs_yaml, quiet_logger, lhs_yaml_file)

mc = MergerConfig(
quiet_logger,
SimpleNamespace(config=config_file),
rules={"/hash/merge_targets/subarray": override_rule}
)
mc.prepare(lhs_data)

node = lhs_data["hash"]["merge_targets"]["subarray"]
parent = lhs_data["hash"]["merge_targets"]
parentref = "subarray"

assert mc.array_merge_mode(
NodeCoords(node, parent, parentref)) == mode

@pytest.mark.parametrize("arg_rule, override_rule, mode", [
("left", "right", ArrayMergeOpts.RIGHT),
("right", "unique", ArrayMergeOpts.UNIQUE),
("unique", "all", ArrayMergeOpts.ALL),
("all", "left", ArrayMergeOpts.LEFT),
])
def test_array_merge_mode_override_rule_overrides_arg_rule(
self, quiet_logger, tmp_path_factory, arg_rule, override_rule, mode
):
lhs_yaml_file = create_temp_yaml_file(tmp_path_factory, """---
hash:
lhs_exclusive: lhs value 1
merge_targets:
subkey: lhs value 2
subarray:
- one
- two
array_of_hashes:
- name: LHS Record 1
id: 1
prop: LHS value AoH 1
- name: LHS Record 2
id: 2
prop: LHS value AoH 2
""")
lhs_yaml = get_yaml_editor()
(lhs_data, lhs_loaded) = get_yaml_data(lhs_yaml, quiet_logger, lhs_yaml_file)

mc = MergerConfig(
quiet_logger,
SimpleNamespace(arrays=arg_rule),
rules={"/hash/merge_targets/subarray": override_rule}
)
mc.prepare(lhs_data)

node = lhs_data["hash"]["merge_targets"]["subarray"]
parent = lhs_data["hash"]["merge_targets"]
parentref = "subarray"

assert mc.array_merge_mode(
NodeCoords(node, parent, parentref)) == mode

###
# aoh_merge_mode
Expand Down Expand Up @@ -419,6 +584,95 @@ def test_aoh_merge_mode_ini_rule_overrides_cli(
assert mc.aoh_merge_mode(
NodeCoords(node, parent, parentref)) == mode

@pytest.mark.parametrize("ini_rule, override_rule, mode", [
("deep", "left", AoHMergeOpts.LEFT),
("left", "right", AoHMergeOpts.RIGHT),
("right", "unique", AoHMergeOpts.UNIQUE),
("unique", "all", AoHMergeOpts.ALL),
("all", "deep", AoHMergeOpts.DEEP),
])
def test_array_merge_mode_override_rule_overrides_ini_rule(
self, quiet_logger, tmp_path_factory, ini_rule, override_rule, mode
):
config_file = create_temp_yaml_file(tmp_path_factory, """
[rules]
/array_of_hashes = {}
""".format(ini_rule))
lhs_yaml_file = create_temp_yaml_file(tmp_path_factory, """---
hash:
lhs_exclusive: lhs value 1
merge_targets:
subkey: lhs value 2
subarray:
- one
- two
array_of_hashes:
- name: LHS Record 1
id: 1
prop: LHS value AoH 1
- name: LHS Record 2
id: 2
prop: LHS value AoH 2
""")
lhs_yaml = get_yaml_editor()
(lhs_data, lhs_loaded) = get_yaml_data(lhs_yaml, quiet_logger, lhs_yaml_file)

mc = MergerConfig(
quiet_logger,
SimpleNamespace(config=config_file),
rules={"/array_of_hashes": override_rule}
)
mc.prepare(lhs_data)

node = lhs_data["array_of_hashes"]
parent = lhs_data
parentref = "array_of_hashes"

assert mc.aoh_merge_mode(
NodeCoords(node, parent, parentref)) == mode

@pytest.mark.parametrize("arg_rule, override_rule, mode", [
("deep", "left", AoHMergeOpts.LEFT),
("left", "right", AoHMergeOpts.RIGHT),
("right", "unique", AoHMergeOpts.UNIQUE),
("unique", "all", AoHMergeOpts.ALL),
("all", "deep", AoHMergeOpts.DEEP),
])
def test_array_merge_mode_override_rule_overrides_arg_rule(
self, quiet_logger, tmp_path_factory, arg_rule, override_rule, mode
):
lhs_yaml_file = create_temp_yaml_file(tmp_path_factory, """---
hash:
lhs_exclusive: lhs value 1
merge_targets:
subkey: lhs value 2
subarray:
- one
- two
array_of_hashes:
- name: LHS Record 1
id: 1
prop: LHS value AoH 1
- name: LHS Record 2
id: 2
prop: LHS value AoH 2
""")
lhs_yaml = get_yaml_editor()
(lhs_data, lhs_loaded) = get_yaml_data(lhs_yaml, quiet_logger, lhs_yaml_file)

mc = MergerConfig(
quiet_logger,
SimpleNamespace(aoh=arg_rule),
rules={"/array_of_hashes": override_rule}
)
mc.prepare(lhs_data)

node = lhs_data["array_of_hashes"]
parent = lhs_data
parentref = "array_of_hashes"

assert mc.aoh_merge_mode(
NodeCoords(node, parent, parentref)) == mode

###
# aoh_merge_key
Expand Down Expand Up @@ -526,6 +780,40 @@ def test_aoh_merge_key_ini_inferred_parent(
assert mc.aoh_merge_key(
NodeCoords(node, parent, parentref), record) == "prop"

def test_aoh_merge_key_override_rule_overrides_ini(self, quiet_logger, tmp_path_factory):
config_file = create_temp_yaml_file(tmp_path_factory, """
[keys]
/array_of_hashes = name
""")
lhs_yaml_file = create_temp_yaml_file(tmp_path_factory, """---
hash:
lhs_exclusive: lhs value 1
merge_targets:
subkey: lhs value 2
subarray:
- one
- two
array_of_hashes:
- name: LHS Record 1
id: 1
prop: LHS value AoH 1
- name: LHS Record 2
id: 2
prop: LHS value AoH 2
""")
lhs_yaml = get_yaml_editor()
(lhs_data, lhs_loaded) = get_yaml_data(lhs_yaml, quiet_logger, lhs_yaml_file)

mc = MergerConfig(quiet_logger, SimpleNamespace(config=config_file), keys={"/array_of_hashes": "id"})
mc.prepare(lhs_data)

node = lhs_data["array_of_hashes"]
parent = lhs_data
parentref = "array_of_hashes"
record = node[0]

assert mc.aoh_merge_key(
NodeCoords(node, parent, parentref), record) == "id"

###
# set_merge_mode
Expand Down
33 changes: 26 additions & 7 deletions yamlpath/merger/mergerconfig.py
View file
Open in desktop
leviem1 marked this conversation as resolved.
Show resolved Hide resolved
leviem1 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
Copyright 2020, 2021 William W. Kimball, Jr. MBA MSIS
"""
import configparser
from typing import Any, Dict, Union
from typing import Any, Dict, Optional
from argparse import Namespace

from yamlpath.exceptions import YAMLPathException
Expand All @@ -24,23 +24,35 @@
class MergerConfig:
"""Config file processor for the Merger."""

def __init__(self, logger: ConsolePrinter, args: Namespace) -> None:
def __init__(
self,
logger: ConsolePrinter,
args: Namespace,
**kwargs: Any,
) -> None:
"""
Instantiate this class into an object.

Parameters:
1. logger (ConsolePrinter) Instance of ConsoleWriter or subclass
2. args (dict) Default options for merge rules
3. kwargs (dict) Overrides for config values

Returns: N/A
"""
self.log = logger
self.args = args
self.config: Union[None, configparser.ConfigParser] = None
self.config: Optional[configparser.ConfigParser] = None
self.rules: Dict[NodeCoords, str] = {}
self.keys: Dict[NodeCoords, str] = {}
config_overrides: Dict[str, Any] = {}

if "keys" in kwargs:
config_overrides["keys"] = kwargs.pop("keys")
if "rules" in kwargs:
config_overrides["rules"] = kwargs.pop("rules")

self._load_config()
self._load_config(config_overrides)

def anchor_merge_mode(self) -> AnchorConflictResolutions:
"""
Expand Down Expand Up @@ -322,7 +334,7 @@ def _prepare_user_rules(
"... NODE:", data=node_coord,
prefix="MergerConfig::_prepare_user_rules: ")

def _load_config(self) -> None:
def _load_config(self, config_overrides: Dict[str, Any]) -> None:
"""Load the external configuration file."""
config = configparser.ConfigParser()

Expand All @@ -334,8 +346,15 @@ def _load_config(self) -> None:

if config_file:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed the overrides will only apply when a config file has been provided. Wasn't the original goal to entirely avoid using an external config file for API users? Using an override mechanism can get us there, but only if it also handles the case of there being no config file to start with. Having said that, such a change blows open the scope of this change because the change would have to also cover the rest of the interface provided by ConfigParser (like .sections(), et al).

This feels like we're missing the goal. Will you be happy with the compromise you've added here or should we dig deeper into this to provide a solution that actually makes the config file optional?

Copy link
Contributor Author

@leviem1 leviem1 Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a case of not fixing an indent after making a change. Everything should work just fine by un-indenting those lines because even if the config file doesn't exist, we still use ConfigParser, adding it as a class member only if we added the overrides to it (by using sections()).

Though your comment made me realize that my tests should have caught this earlier, so I checked out my tests and realized that anything specified in self.args has a higher priority than my changes, which I think misses the mark. I'll take a deeper look into fixing this when I have a bit more time to update everything, but I'll push some changes to the tests soon to illustrate the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disregard that last part, turns out it works just fine based on the tests I added. I added a fix and some tests for the fix.

config.read(config_file)
if config.sections():
self.config = config

if "keys" in config_overrides:
config["keys"] = config_overrides["keys"]

if "rules" in config_overrides:
config["rules"] = config_overrides["rules"]

if config.sections():
self.config = config

def _get_config_for(self, node_coord: NodeCoords, section: dict) -> str:
"""
Expand Down