Skip to content

Follow-up: reconsider bool / int return on Remove* methods (dime T4) #247

Description

@ottobolyos

Origin

Dime Ultrareview cycle-1 on PR #241 (fix/device-validation-level-hardening), tracked finding T4 — surfaced by the improvement agent.

Finding

The four `Remove*(string id)` methods on `Device` and `Component` — `RemoveComponent`, `RemoveComposition`, `RemoveDataItem` — all return `void`. Callers cannot tell:

  • whether the requested id was present in the tree at all (removal was a no-op),
  • how many nodes matched (a duplicated id shows up as more than one removal),
  • whether the tree shape actually changed.

For `MTConnectAgent.NormalizeDevice`'s Remove branch, this matters — the validation handler fires the `Invalid*Removed` event based on the assumption that the entity was present. If a race deleted the id concurrently, the event fires spuriously.

Rationale for defer

Changing the return type from `void` to `bool` or `int` is binary-API-breaking — every third-party consumer that has a Component-tree walker will see a signature change. It cannot land in the same PR as a bug fix that must be back-portable to point releases; it needs its own major-version cycle plus an explicit deprecation window.

Suggested phased design (for the follow-up PR)

  1. Add new overloads: `bool TryRemoveComponent(string, out int removedCount)` etc., leaving the existing `void Remove*` in place.
  2. Mark the `void` overloads `[Obsolete("Use TryRemove* overloads for removal reporting", error: false)]` in the next minor release.
  3. In the following major release, remove the `void` overloads and promote `TryRemove*` to the canonical name.

Refs: dime cycle-1 T4 (improvement) — see PR #241's H1 recursion fix, which is the immediate trigger for wanting the removal count surfaced.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions