Stop allowing hyphens in metric names and add name validation tests

[theyostalservice]

Problem

Problems:

1. The validation for metric names should not allow hyphens. They are already disallowed in dbt-semantic-interfaces and metricflow, and the error message here correctly identifies that they're not allowed, but a mistake in the regex allows them to slip through.
2. These name errors are validation errors, not parsing errors, and should be treated as such.
3. We don't have any tests showing that these work and protecting them during maintenance.

Solution

Fix the regex, change the error type that we raise, and add tests!

Checklist

• I have read the contributing guide and understand what's expected of me.
• I have run this code in development, and it appears to resolve the stated issue.
• This PR includes tests, or tests are not required or relevant for this PR.
• This PR has no interface changes (e.g., macros, CLI, logs, JSON artifacts, config files, adapter interface, etc.) or this PR has already received feedback and approval from Product or DX.
• This PR includes type annotations for new and modified functions.

[theyostalservice]

• Add new-syle YAML metric #12157
• Stop allowing hyphens in metric names and add name validation tests #12158 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

[github-actions]

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.93%. Comparing base (11ada88) to head (ef95e92).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12158      +/-   ##
==========================================
- Coverage   91.98%   91.93%   -0.05%     
==========================================
  Files         203      203              
  Lines       24847    24847              
==========================================
- Hits        22856    22844      -12     
- Misses       1991     2003      +12     

Flag	Coverage Δ
integration	88.81% <100.00%> (-0.12%)	⬇️
unit	65.20% <100.00%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
Unit Tests	65.20% <100.00%> (+0.02%)	⬆️
Integration Tests	88.81% <100.00%> (-0.12%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[theyostalservice]

tbh, I can't help but think it might make more sense to try to unify this regex and pull it directly from dbt-semantic-interfaces, but with all the changes afoot with that repo and metricflow right now, it seems better to avoid the added workflow complexity.

But, for reference, here is the different but definitely-not-allowing hyphens regex fron dsi:

    NAME_REGEX = re.compile(r"\A[a-z]((?!__)[a-z0-9_])*[a-z0-9]\Z")

    @staticmethod
    def check_valid_name(  # noqa: D
        name: str, context: Optional[ValidationContext] = None
    ) -> Sequence[ValidationIssue]:
        issues: List[ValidationIssue] = []

        if not UniqueAndValidNameRule.NAME_REGEX.match(name):
            issues.append(
                ValidationError(
                    context=context,
                    message=f"Invalid name `{name}` - names may only contain lower case letters, numbers, "
                    f"and underscores. Additionally, names must start with a lower case letter, cannot end "
                    f"with an underscore, cannot contain dunders (double underscores, or __), and must be "
                    f"at least 2 characters long.",
                )
            )

[QMalcolm]

@theyostalservice is my understanding correct that these were raising an error previously on the MetricFlow side, but just not in core? I ask because if so, this should be good as is. If no error was being raised anywhere then we'd have to do some additional handling in code as it'd be a breaking change

[theyostalservice]

Yes, that's the correct understanding! :-)