Fix plural of "partial success"

[jtcohen6]

Resolves #10999

Problem

The plural of partial success

Solution

Implement PartialSuccess.pluralize() (sort of)

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.

[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 89.08%. Comparing base (6e1f64f) to head (0f935aa).
Report is 17 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11002      +/-   ##
==========================================
- Coverage   89.12%   89.08%   -0.05%     
==========================================
  Files         183      183              
  Lines       23626    23646      +20     
==========================================
+ Hits        21057    21065       +8     
- Misses       2569     2581      +12     

Flag	Coverage Δ
integration	86.40% <ø> (-0.09%)	⬇️
unit	62.82% <ø> (+0.03%)	⬆️

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

Components	Coverage Δ
Unit Tests	62.82% <ø> (+0.03%)	⬆️
Integration Tests	86.40% <ø> (-0.09%)	⬇️

[jtcohen6]

This is likely overdoing it. Alternative ideas:

• implement NodeStatus.pluralize(), and import/use the actual NodeStatus enum here
• just reimplement the logic of pluralize (if count != 1) for this particular string, instead of calling pluralize to create partial_success_plural

[QMalcolm]

I agree. I feel like creating a PartialSuccess class here feels out of place 🤔 Out of the alternatives you've listed, I think the first would be preferable

[jtcohen6]

I started down this path, but two things are giving me pause:

• I would need to make this a nested import, to avoid a circular import (since dbt.artifacts.schemas.results imports the event system)
• NodeStatus.Warn has its string type set to `"warn", and here we have "warning(s)"

class NodeStatus(StrEnum):
    Success = "success"
    Error = "error"
    Fail = "fail"
    Warn = "warn"
    Skipped = "skipped"
    PartialSuccess = "partial success"
    Pass = "pass"
    RuntimeErr = "runtime error"

    def pluralize(self) -> str:
        if self is self.Success or self is self.PartialSuccess or self is self.Pass:
            return f"{self}es"
        elif self is self.Warn:
            return f"warnings" # feels inconsistent
        elif self is self.Skipped:
            return "skipped"
        return f"{self}s"

        from dbt.artifacts.schemas.results import NodeStatus

        error_plural = pluralize(self.num_errors, NodeStatus.Error)
        warn_plural = pluralize(self.num_warnings, NodeStatus.Warn)
        partial_success_plural = pluralize(self.num_partial_success, NodeStatus.PartialSuccess)

[QMalcolm]

I would need to make this a nested import, to avoid a circular import (since dbt.artifacts.schemas.results imports the event system)

🫠 Right, we haven't finished the decoupling. Woof okay. I'll look in a bit and see if we can break the circular import on the artifacts side.

[dbeatty10]

Are we sure that this doesn't involve the definition of pluralize from dbt-common?

https://github.com/dbt-labs/dbt-common/blob/60ffb0689a41f19f84f7c8f97cddd16d7f0fc732/dbt_common/events/format.py#L40-L51

dbt-core/core/dbt/events/types.py

Lines 12 to 14 in a86e2b4

	from dbt_common.events.format import (
	format_fancy_output_line,
	pluralize,

[jtcohen6]

@dbeatty10 It does! That's the pluralize method in the code right now

That method says:

• If there's exactly 1 of me, return me (unchanged)
• If there's != 1 of me, and I'm being passed a string argument that is actually a class which implements a method named pluralize, then use that method to pluralize me; otherwise, just add s

So the question here is:

• Should we implement a one-off class for PartialSuccess within EndOfRunSummary that implements pluralize?
• Do I use the actual NodeStatus class here, and implement pluralize? (problems described above)
• Do I just ignore dbt_common.events.format.pluralize, and reimplement that if count !=1 logic within EndOfRunSummary, for "partial success" only?

        error_plural = pluralize(self.num_errors, "error")
        warn_plural = pluralize(self.num_warnings, "warning")
        partial_success_plural = f"""{self.num_partial_success} {"partial success" if self.num_partial_success == 1 else "partial successes"}"""

[dbeatty10]

       partial_success_plural = f"""{self.num_partial_success} {"partial success" if self.num_partial_success == 1 else "partial successes"}"""

is ugly, but seems like the fastest and easiest way to solve this in a targeted manner.

If we choose that option, it would solve the problem now, then we could create a tech debt issue to come back and convert it back to pluralize once the decoupling part is done.

Obviously totally up to @QMalcolm -- just throwing out some ideass.

[jtcohen6]

print_run_end_messages() expects this result to have an actual node object, with a unique_id value and a node_info key

[jtcohen6]

Thank you @MichelleArk @QMalcolm !