Skip to content

Commit

Permalink
fixup! issue #76 report missing parameter description
Browse files Browse the repository at this point in the history
  • Loading branch information
@soxofaan
soxofaan committed Nov 24, 2022
1 parent 93489c1 commit fd6a3d7
Show file tree
Hide file tree
Showing 2 changed files with 163 additions and 39 deletions.
37 changes: 21 additions & 16 deletions src/openeo_aggregator/metadata/merging.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -374,8 +374,11 @@ def _merge_process_parameters(
params = process_metadata.get("parameters", [])
if params:
normalizer = ProcessParameterNormalizer(
backend_id, process_id,
strip_description=False, add_optionals=False
strip_description=False,
add_optionals=False,
report=functools.partial(
self.report, backend_id=backend_id, process_id=process_id
),
)
merged = normalizer.normalize_parameters(params)

Expand Down Expand Up @@ -408,9 +411,11 @@ def _merge_process_parameters(
)
for name in set(merged_params_by_name).intersection(params_by_name):
normalizer = ProcessParameterNormalizer(
backend_id, process_id,
strip_description=True,
add_optionals=True,
report=functools.partial(
self.report, backend_id=backend_id, process_id=process_id
),
)
merged_param = normalizer.normalize_parameter(
merged_params_by_name[name]
Expand Down Expand Up @@ -505,33 +510,33 @@ class ProcessParameterNormalizer:
e.g. for comparison purposes.
"""

__slots__ = ["backend_id", "process_id", "strip_description", "add_optionals", "report"]
__slots__ = ["strip_description", "add_optionals", "report"]

def __init__(self, backend_id, process_id,
strip_description: bool = False, add_optionals: bool = True,
report: Callable = DEFAULT_REPORTER.report):
self.backend_id = backend_id
self.process_id = process_id
def __init__(
self,
strip_description: bool = False,
add_optionals: bool = True,
report: Callable = DEFAULT_REPORTER.report,
):
self.strip_description = strip_description
self.add_optionals = add_optionals
self.report = report

def normalize_parameter(self, param: dict) -> dict:
"""Normalize a parameter metadata dict"""
# TODO: report missing name/description/schema?
for required in ["name", "schema", "description"]:
if required not in param:
self.report(
f"Missing required field {required!r} in parameter metadata {param!r}"
)
normalized = {
"name": param.get("name", "n/a"),
"schema": param.get("schema", {}),
}
if self.strip_description:
normalized["description"] = "-"
else:
normalized["description"] = param.get("description", "")
if normalized["description"] == "":
self.report("Missing description for parameter",
backend_id = self.backend_id, process_id = self.process_id, parameter = normalized["name"]
)
normalized["description"] = normalized["name"]
normalized["description"] = param.get("description", normalized["name"])
for field, default_value in [
("optional", False),
("deprecated", False),
Expand Down
165 changes: 142 additions & 23 deletions tests/metadata/test_merging.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -267,8 +267,16 @@ def test_merge_process_parameters_basic(self, merger, reporter):
spec = {
"id": "add",
"parameters": [
{"name": "x", "schema": {"type": "number"}},
{"name": "y", "schema": {"type": "number"}},
{
"name": "x",
"schema": {"type": "number"},
"description": "The x value.",
},
{
"name": "y",
"schema": {"type": "number"},
"description": "The y value.",
},
],
}
result = merger.merge_process_metadata(
Expand All @@ -282,8 +290,16 @@ def test_merge_process_parameters_basic(self, merger, reporter):
"id": "add",
"description": "add",
"parameters": [
{"name": "x", "description": "x", "schema": {"type": "number"}},
{"name": "y", "description": "y", "schema": {"type": "number"}},
{
"name": "x",
"description": "The x value.",
"schema": {"type": "number"},
},
{
"name": "y",
"description": "The y value.",
"schema": {"type": "number"},
},
],
"returns": {"schema": {}},
"federation:backends": ["b1", "b2"],
Expand All @@ -299,7 +315,13 @@ def test_merge_process_parameters_invalid(self, merger, reporter):
{
"b1": {
"id": "cos",
"parameters": [{"name": "x", "schema": {"type": "number"}}],
"parameters": [
{
"name": "x",
"schema": {"type": "number"},
"description": "x value",
}
],
},
"b2": {"id": "cos", "parameters": [1324]},
}
Expand All @@ -310,13 +332,13 @@ def test_merge_process_parameters_invalid(self, merger, reporter):
"federation:backends": ["b1", "b2"],
"id": "cos",
"parameters": [
{"description": "x", "name": "x", "schema": {"type": "number"}}
{"name": "x", "schema": {"type": "number"}, "description": "x value"}
],
"returns": {"schema": {}},
'deprecated': False,
'experimental': False,
'examples': [],
'links': []
"deprecated": False,
"experimental": False,
"examples": [],
"links": [],
}
assert reporter.logs == [
{
Expand All @@ -334,12 +356,77 @@ def test_merge_process_parameters_invalid(self, merger, reporter):
},
]

def test_merge_process_parameters_missing_required(self, merger, reporter):
result = merger.merge_process_metadata(
{
"b1": {
"id": "cos",
"parameters": [
{
"name": "x",
"schema": {"type": "number"},
"description": "x value",
}
],
},
"b2": {"id": "cos", "parameters": [{"name": "x"}]},
}
)

assert result == {
"description": "cos",
"federation:backends": ["b1", "b2"],
"id": "cos",
"parameters": [
{"name": "x", "schema": {"type": "number"}, "description": "x value"}
],
"returns": {"schema": {}},
"deprecated": False,
"experimental": False,
"examples": [],
"links": [],
}
assert reporter.logs == [
{
"backend_id": "b2",
"msg": "Missing required field 'schema' in parameter metadata {'name': 'x'}",
"process_id": "cos",
},
{
"backend_id": "b2",
"msg": "Missing required field 'description' in parameter metadata {'name': 'x'}",
"process_id": "cos",
},
{
"backend_id": "b2",
"msg": "Parameter 'x' field 'schema' value differs from merged.",
"diff": [
"--- merged\n",
"+++ b2\n",
"@@ -1,3 +1 @@\n",
"-{\n",
'- "type": "number"\n',
"-}\n",
"+{}\n",
],
"merged": {"type": "number"},
"process_id": "cos",
"value": {},
},
]

def test_merge_process_parameters_invalid_listing(self, merger, reporter):
result = merger.merge_process_metadata(
{
"b1": {
"id": "cos",
"parameters": [{"name": "x", "schema": {"type": "number"}}],
"parameters": [
{
"name": "x",
"schema": {"type": "number"},
"description": "x value",
}
],
},
"b2": {"id": "cos", "parameters": 1324},
}
Expand All @@ -350,7 +437,7 @@ def test_merge_process_parameters_invalid_listing(self, merger, reporter):
"federation:backends": ["b1", "b2"],
"id": "cos",
"parameters": [
{"description": "x", "name": "x", "schema": {"type": "number"}}
{"name": "x", "schema": {"type": "number"}, "description": "x value"}
],
"returns": {"schema": {}},
'deprecated': False,
Expand Down Expand Up @@ -379,15 +466,18 @@ def test_merge_process_parameters_differences(self, merger, reporter):
"b1": {
"id": "cos",
"parameters": [
{"name": "x", "schema": {"type": "number"}},
{
"name": "x",
"schema": {"type": "number"},
"description": "The X value.",
},
],
},
"b2": {
"id": "cos",
"parameters": [
{
"name": "x",
"description": "X value",
"schema": {"type": "number"},
"deprecated": False,
"optional": False,
Expand All @@ -406,7 +496,11 @@ def test_merge_process_parameters_differences(self, merger, reporter):
"id": "cos",
"description": "cos",
"parameters": [
{"description": "x", "name": "x", "schema": {"type": "number"}}
{
"name": "x",
"schema": {"type": "number"},
"description": "The X value.",
}
],
"returns": {"schema": {}},
"federation:backends": ["b1", "b2", "b3"],
Expand All @@ -416,6 +510,16 @@ def test_merge_process_parameters_differences(self, merger, reporter):
'links': [],
}
assert reporter.logs == [
{
"backend_id": "b2",
"msg": "Missing required field 'description' in parameter metadata {'name': 'x', 'schema': {'type': 'number'}, 'deprecated': False, 'optional': False}",
"process_id": "cos",
},
{
"backend_id": "b3",
"msg": "Missing required field 'description' in parameter metadata {'name': 'x', 'schema': {'type': 'array'}}",
"process_id": "cos",
},
{
"msg": "Parameter 'x' field 'schema' value differs from merged.",
"backend_id": "b3",
Expand Down Expand Up @@ -445,10 +549,13 @@ def test_merge_process_parameters_recursive(self, merger, reporter):
"parameters": [
{
"name": "process",
"description": "Callback",
"schema": {
"type": "object",
"subtype": "process-graph",
"parameters": [{"name": "x", "schema": {}}],
"parameters": [
{"name": "x", "schema": {}, "description": "the x"}
],
"returns": {"schema": {}},
},
},
Expand All @@ -459,13 +566,15 @@ def test_merge_process_parameters_recursive(self, merger, reporter):
"parameters": [
{
"name": "process",
"description": "A Callback",
"schema": {
"type": "object",
"subtype": "process-graph",
"parameters": [
{
"name": "x",
"schema": {},
"description": "The x",
"optional": False,
"deprecated": False,
"experimental": False,
Expand All @@ -487,11 +596,13 @@ def test_merge_process_parameters_recursive(self, merger, reporter):
"parameters": [
{
"name": "process",
"description": "process",
"description": "Callback",
"schema": {
"type": "object",
"subtype": "process-graph",
"parameters": [{"name": "x", "description": "x", "schema": {}}],
"parameters": [
{"name": "x", "description": "the x", "schema": {}}
],
"returns": {"schema": {}},
},
}
Expand All @@ -514,12 +625,19 @@ def test_merge_process_parameters_recursive2(self, merger, reporter):
"b1": {
"id": "count",
"parameters": [
{"name": "data", "schema": {"type": "array"}},
{
"name": "data",
"schema": {"type": "array"},
"description": "Data",
},
{
"name": "condition",
"description": "what to count",
"schema": [
{
"parameters": [{"name": "x", "schema": {}}],
"parameters": [
{"name": "x", "schema": {}, "description": "X."}
],
"returns": {"schema": {"type": "boolean"}},
"subtype": "process-graph",
"type": "object",
Expand Down Expand Up @@ -548,6 +666,7 @@ def test_merge_process_parameters_recursive2(self, merger, reporter):
},
{
"name": "condition",
"description": "Condition callback.",
"deprecated": False,
"experimental": False,
"optional": True,
Expand Down Expand Up @@ -581,18 +700,18 @@ def test_merge_process_parameters_recursive2(self, merger, reporter):
"description": "count",
"federation:backends": ["b1", "b2"],
"parameters": [
{"name": "data", "description": "data", "schema": {"type": "array"}},
{"name": "data", "description": "Data", "schema": {"type": "array"}},
{
"name": "condition",
"description": "condition",
"description": "what to count",
"default": None,
"optional": True,
"schema": [
{
"type": "object",
"subtype": "process-graph",
"parameters": [
{"name": "x", "description": "x", "schema": {}}
{"name": "x", "description": "X.", "schema": {}}
],
"returns": {"schema": {"type": "boolean"}},
},
Expand Down

0 comments on commit fd6a3d7

Please sign in to comment.