Repository: canonical/charmcraft

Issue:
All linters should produce an error text in function to what they found
If the linter finds a problem in a file, the error text should not be static (JujuMetadata currently only produces `Problems found with metadata.yaml file.`) but dinamically according to what the problem is.

Discussion:

Patch:
diff --git a/charmcraft/linters.py b/charmcraft/linters.py
index 86c28db8..8c008dad 100644
--- a/charmcraft/linters.py
+++ b/charmcraft/linters.py
@@ -1,4 +1,4 @@
-# Copyright 2021 Canonical Ltd.
+# Copyright 2021-2022 Canonical Ltd.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -25,8 +25,8 @@ from typing import List, Generator, Union
 
 import yaml
 
-from charmcraft import config
-from charmcraft.metadata import parse_metadata_yaml
+from charmcraft import config, utils
+from charmcraft.metadata import parse_metadata_yaml, read_metadata_yaml
 
 CheckType = namedtuple("CheckType", "attribute lint")(attribute="attribute", lint="lint")
 
@@ -215,25 +215,32 @@ class JujuMetadata:
     check_type = CheckType.lint
     name = "metadata"
     url = "https://juju.is/docs/sdk/charmcraft-analyze#heading--metadata"
-    text = "Problems found with metadata.yaml file."
 
     # different result constants
     Result = namedtuple("Result", "ok errors")(ok=OK, errors=ERRORS)
 
+    def __init__(self):
+        self.text = None
+
     def run(self, basedir: pathlib.Path) -> str:
         """Run the proper verifications."""
         try:
-            metadata = parse_metadata_yaml(basedir)
+            metadata = read_metadata_yaml(basedir)
+        except yaml.YAMLError:
+            self.text = "The metadata.yaml file is not a valid YAML file."
+            return self.Result.errors
         except Exception:
-            # file not found, corrupted, or mandatory "name" not present
+            self.text = "Cannot read the metadata.yaml file."
             return self.Result.errors
 
-        # no need to verify "name" as it's mandatory in the metadata parsing
-        if metadata.summary and metadata.description:
-            result = self.Result.ok
-        else:
-            result = self.Result.errors
-        return result
+        # check required attributes
+        missing_fields = {"name", "summary", "description"} - set(metadata)
+        if missing_fields:
+            missing = utils.humanize_list(missing_fields, "and")
+            self.text = f"The metadata.yaml file is missing the following attribute(s): {missing}."
+            return self.Result.errors
+
+        return self.Result.ok
 
 
 class JujuActions:
diff --git a/charmcraft/metadata.py b/charmcraft/metadata.py
index 5b554ff3..92601807 100644
--- a/charmcraft/metadata.py
+++ b/charmcraft/metadata.py
@@ -1,4 +1,4 @@
-# Copyright 2020-2021 Canonical Ltd.
+# Copyright 2020-2022 Canonical Ltd.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -49,6 +49,17 @@ class CharmMetadata(pydantic.BaseModel, frozen=True, validate_all=True):
             raise CraftError(format_pydantic_errors(error.errors(), file_name=CHARM_METADATA))
 
 
+def read_metadata_yaml(charm_dir: pathlib.Path) -> Any:
+    """Parse project's metadata.yaml.
+
+    :returns: the YAML decoded metadata.yaml content
+    """
+    metadata_path = charm_dir / CHARM_METADATA
+    emit.trace(f"Reading {str(metadata_path)!r}")
+    with metadata_path.open("rt", encoding="utf8") as fh:
+        return yaml.safe_load(fh)
+
+
 def parse_metadata_yaml(charm_dir: pathlib.Path) -> CharmMetadata:
     """Parse project's metadata.yaml.
 
@@ -56,12 +67,10 @@ def parse_metadata_yaml(charm_dir: pathlib.Path) -> CharmMetadata:
 
     :raises: CraftError if metadata does not exist.
     """
-    metadata_path = charm_dir / CHARM_METADATA
-    emit.trace(f"Parsing {str(metadata_path)!r}")
+    try:
+        metadata = read_metadata_yaml(charm_dir)
+    except OSError as exc:
+        raise CraftError(f"Cannot read the metadata.yaml file: {exc!r}") from exc
 
-    if not metadata_path.exists():
-        raise CraftError("Missing mandatory metadata.yaml.")
-
-    with metadata_path.open("rt", encoding="utf8") as fh:
-        metadata = yaml.safe_load(fh)
-        return CharmMetadata.unmarshal(metadata)
+    emit.trace("Validating metadata format")
+    return CharmMetadata.unmarshal(metadata)
diff --git a/charmcraft/utils.py b/charmcraft/utils.py
index a5fe347a..9474b526 100644
--- a/charmcraft/utils.py
+++ b/charmcraft/utils.py
@@ -25,6 +25,7 @@ import time
 from collections import namedtuple
 from dataclasses import dataclass
 from stat import S_IRGRP, S_IROTH, S_IRUSR, S_IXGRP, S_IXOTH, S_IXUSR
+from typing import Iterable
 
 import yaml
 from craft_cli import emit, CraftError
@@ -243,3 +244,18 @@ def format_timestamp(dt: datetime.datetime) -> str:
     # convert to UTC from whatever timezone `dt` has
     dtz = datetime.datetime.fromtimestamp(time.mktime(dt.utctimetuple()))
     return dtz.strftime("%Y-%m-%dT%H:%M:%SZ")
+
+
+def humanize_list(items: Iterable[str], conjunction: str) -> str:
+    """Format a list into a human-readable string.
+
+    :param items: list to humanize, must not be empty
+    :param conjunction: the conjunction used to join the final element to
+                        the rest of the list (e.g. 'and').
+    """
+    if not items:
+        raise ValueError("Cannot humanize an empty list.")
+    *initials, final = map(repr, sorted(items))
+    if not initials:
+        return final
+    return f"{', '.join(initials)} {conjunction} {final}"


Test patch:
diff --git a/tests/commands/test_build.py b/tests/commands/test_build.py
index 81efb555..9354b0a0 100644
--- a/tests/commands/test_build.py
+++ b/tests/commands/test_build.py
@@ -542,7 +542,10 @@ def test_build_error_without_metadata_yaml(basic_project, monkeypatch):
     config = load(basic_project)
     monkeypatch.chdir(basic_project)
 
-    with pytest.raises(CraftError, match=r"Missing mandatory metadata.yaml."):
+    msg = re.escape(
+        "Cannot read the metadata.yaml file: FileNotFoundError(2, 'No such file or directory')"
+    )
+    with pytest.raises(CraftError, match=msg):
         get_builder(config)
 
 
diff --git a/tests/test_linters.py b/tests/test_linters.py
index 4a9430e0..db3ce4eb 100644
--- a/tests/test_linters.py
+++ b/tests/test_linters.py
@@ -1,4 +1,4 @@
-# Copyright 2021 Canonical Ltd.
+# Copyright 2021-2022 Canonical Ltd.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -515,52 +515,59 @@ def test_jujumetadata_all_ok(tmp_path):
 
 def test_jujumetadata_missing_file(tmp_path):
     """No metadata.yaml file at all."""
-    result = JujuMetadata().run(tmp_path)
+    linter = JujuMetadata()
+    result = linter.run(tmp_path)
     assert result == JujuMetadata.Result.errors
+    assert linter.text == "Cannot read the metadata.yaml file."
 
 
 def test_jujumetadata_file_corrupted(tmp_path):
     """The metadata.yaml file is not valid YAML."""
     metadata_file = tmp_path / "metadata.yaml"
     metadata_file.write_text(" - \n-")
-    result = JujuMetadata().run(tmp_path)
+    linter = JujuMetadata()
+    result = linter.run(tmp_path)
     assert result == JujuMetadata.Result.errors
+    assert linter.text == "The metadata.yaml file is not a valid YAML file."
 
 
-def test_jujumetadata_missing_name(tmp_path):
-    """A required "name" is missing in the metadata file."""
+_mandatory_fields = ["summary", "description", "name"]
+
+
+@pytest.mark.parametrize("to_miss", range(len(_mandatory_fields)))
+def test_jujumetadata_missing_field_simple(tmp_path, to_miss):
+    """A required field is missing in the metadata file."""
+    included_fields = _mandatory_fields.copy()
+    missing = included_fields.pop(to_miss)
+
     # metadata file with not all fields
     metadata_file = tmp_path / "metadata.yaml"
-    metadata_file.write_text(
-        """
-        summary: Small text.
-        description: Lot of text.
-    """
-    )
-    result = JujuMetadata().run(tmp_path)
+    content = "\n".join(f"{field}: some text" for field in included_fields)
+    metadata_file.write_text(content)
+    linter = JujuMetadata()
+    result = linter.run(tmp_path)
     assert result == JujuMetadata.Result.errors
+    assert linter.text == (
+        f"The metadata.yaml file is missing the following attribute(s): '{missing}'."
+    )
 
 
-@pytest.mark.parametrize(
-    "fields",
-    [
-        """
-    name: foobar
-    summary: Small text.
-    """,
-        """
-    name: foobar
-    description: Lot of text.
-    """,
-    ],
-)
-def test_jujumetadata_missing_field(tmp_path, fields):
-    """A required field is missing in the metadata file."""
+def test_jujumetadata_missing_field_multiple(tmp_path):
+    """More than one required field is missing in the metadata file."""
     # metadata file with not all fields
     metadata_file = tmp_path / "metadata.yaml"
-    metadata_file.write_text(fields)
-    result = JujuMetadata().run(tmp_path)
+    metadata_file.write_text(
+        """
+        name: foobar
+    """
+    )
+    linter = JujuMetadata()
+    result = linter.run(tmp_path)
     assert result == JujuMetadata.Result.errors
+    assert linter.text == (
+        "The metadata.yaml file is missing the following attribute(s): "
+        "'description' and 'summary'."
+    )
 
 
 # --- tests for analyze function
diff --git a/tests/test_metadata.py b/tests/test_metadata.py
index 0552db8a..75b1e3e9 100644
--- a/tests/test_metadata.py
+++ b/tests/test_metadata.py
@@ -1,4 +1,4 @@
-# Copyright 2020-2021 Canonical Ltd.
+# Copyright 2020-2022 Canonical Ltd.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -20,7 +20,10 @@ from textwrap import dedent
 import pytest
 from craft_cli import CraftError
 
-from charmcraft.metadata import parse_metadata_yaml
+from charmcraft.metadata import parse_metadata_yaml, read_metadata_yaml
+
+
+# tests for parsing metadata
 
 
 def test_parse_metadata_yaml_complete(tmp_path):
@@ -66,5 +69,40 @@ def test_parse_metadata_yaml_error_invalid_names(tmp_path, name):
 
 
 def test_parse_metadata_yaml_error_missing(tmp_path):
-    with pytest.raises(CraftError, match=r"Missing mandatory metadata.yaml."):
+    msg = re.escape(
+        "Cannot read the metadata.yaml file: FileNotFoundError(2, 'No such file or directory')"
+    )
+    with pytest.raises(CraftError, match=msg):
         parse_metadata_yaml(tmp_path)
+
+
+# tests for reading metadata raw content
+
+
+def test_read_metadata_yaml_complete(tmp_path):
+    """Example of parsing with all the optional attributes."""
+    metadata_file = tmp_path / "metadata.yaml"
+    metadata_file.write_text(
+        """
+        name: test-name
+        summary: Test summary
+        description: Text.
+    """
+    )
+
+    metadata = read_metadata_yaml(tmp_path)
+    assert metadata == {"name": "test-name", "summary": "Test summary", "description": "Text."}
+
+
+def test_read_metadata_yaml_error_invalid(tmp_path):
+    """Open a metadata.yaml that would fail verification."""
+    metadata_file = tmp_path / "metadata.yaml"
+    metadata_file.write_text("- whatever")
+    metadata = read_metadata_yaml(tmp_path)
+    assert metadata == ["whatever"]
+
+
+def test_read_metadata_yaml_error_missing(tmp_path):
+    """Do not hide the file not being accesible."""
+    with pytest.raises(OSError):
+        read_metadata_yaml(tmp_path)
diff --git a/tests/test_utils.py b/tests/test_utils.py
index 3720840d..cf347444 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -32,6 +32,7 @@ from charmcraft.utils import (
     format_timestamp,
     get_host_architecture,
     get_os_platform,
+    humanize_list,
     load_yaml,
     make_executable,
     useful_filepath,
@@ -438,3 +439,26 @@ def test_timestampstr_nonutc():
     source = dateutil.parser.parse("2020-07-03T20:30:40+03:00")
     result = format_timestamp(source)
     assert result == "2020-07-03T17:30:40Z"
+
+
+# -- tests for humanizing list joins
+
+
+@pytest.mark.parametrize(
+    "items,conjunction,expected",
+    (
+        (["foo"], "xor", "'foo'"),
+        (["foo", "bar"], "xor", "'bar' xor 'foo'"),
+        (["foo", "bar", "baz"], "xor", "'bar', 'baz' xor 'foo'"),
+        (["foo", "bar", "baz", "qux"], "xor", "'bar', 'baz', 'foo' xor 'qux'"),
+    ),
+)
+def test_humanize_list_ok(items, conjunction, expected):
+    """Several successful cases."""
+    assert humanize_list(items, conjunction) == expected
+
+
+def test_humanize_list_empty():
+    """Calling to humanize an empty list is an error that should be explicit."""
+    with pytest.raises(ValueError):
+        humanize_list([], "whatever")