Update Microsoft.ServiceFabric.Data* documentation#604
Draft
olegsych wants to merge 783 commits into
Draft
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #604 +/- ##
========================================
Coverage 49.21% 49.21%
========================================
Files 391 391
Lines 18492 18492
Branches 1845 1845
========================================
Hits 9101 9101
Misses 9075 9075
Partials 316 316
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
olegsych
force-pushed
the
user/olegsych/DataDocs
branch
2 times, most recently
from
364226c to
16c9daa
Compare
Address the following finding.
#### [warn] Should fix TryAddStateSerializer summary should fold in the boolean result and drop `<returns>` (author: Gemini; cross-check: GPT Agree, Opus Agree)
Author (Gemini): TryAddStateSerializer returns a value; per docs.instructions.md the summary should follow "{what it does} and returns {what it returns}" and the <returns> tag should be dropped. Suggested: "Tries to register a custom serializer for all reliable collections and returns true if the custom serializer was added; otherwise, false."
GPT - Agree. TryAddStateSerializer<T> returns bool. The Boolean-return wording rule and the value-returning summary rule both apply.
Opus - Agree. A Try* method returning bool is exactly the TryParse pattern in the docs rule; fold the return into the summary and drop the redundant <returns>.
Address the following finding. #### [⚠️ ](# "Should fix") `CreateTransaction` summary uses imperative voice and a redundant `<returns>` *(authors: Gemini + Opus; cross-check: all Agree)* **Author (Gemini):** Per `docs.instructions.md` "For methods where action and result are logically the same, use template `Returns {what it returns}`." Suggested: "Returns a new `<see cref="ITransaction"/>` that can be used to group operations to be performed atomically." and drop `<returns>`. **Author (Opus):** `CreateTransaction` summary reads "Create and start a new transaction…" using imperative voice; summaries should start with a third-person verb — "Creates and starts…". **GPT — Agree** (both shapes). `CreateTransaction` returns `ITransaction` ([#L69](src/Data.Interfaces/IReliableStateManager.cs#L69)); a factory method is a good fit for the `Returns …` template, and the current `<returns>A new transaction.</returns>` becomes redundant. Caveat: Gemini's "Returns …" shape is more directly aligned with the value-returning factory rule than Opus's "Creates and starts…". **Opus (cross-checking Gemini''s variant) — Agree.** The current verb "Create" violates the present-tense third-person rule; the factory matches the `Parse` "Returns {what it returns}" template and the redundant `<returns>` should be dropped. Caveat: if the "started" state is meaningful to consumers, preserve it (the `<remarks>` covers usage). **Gemini (cross-checking Opus''s variant) — Agree, with caveat.** "Create and start…" doesn''t satisfy the third-person rule; "Creates and starts…" would. Caveat: because this is a value-returning factory, Gemini''s own `Returns a new…` shape aligns more directly with the method-specific rule. > Synthesis: Both variants describe the same fix. Preferred wording is the `Returns a new <see cref="ITransaction"/>…` form (better matches the factory-method rule); preserve the "started" semantics if relevant. Additional changes: reworded the summary to "Returns a new, started <see cref="ITransaction"/> ..." to preserve the "started" semantics, and removed the redundant <returns> element.
Address the following finding. #### [⚠️ ](# "Should fix") String-name overloads document inaccurate `null` behavior and omit reserved-prefix `FormatException` *(author: GPT; cross-check: Gemini Agree, Opus Agree)* **Author (GPT):** The string-name overloads document `ArgumentNullException` when `name` is null across the get/add, remove, and try-get members. In the read-only reference, `ReliableStateManagerImpl.GetUriName(string name)` calls `name.StartsWith("urn:")` before any null validation, so a null string name is not evidenced to throw `ArgumentNullException`. The same helper explicitly throws `FormatException` when the name starts with the reserved `urn:` prefix, but none of the string-overload docs mention it. `docs.instructions.md` requires documenting exceptions with evidence they're actually thrown and the condition under which they're thrown. **Gemini — Agree.** The reference throws `NullReferenceException` (from `name.StartsWith`) instead of the documented `ArgumentNullException` when `name` is null, and throws `FormatException` for the `"urn:"` prefix. The rules mandate documenting exceptions thrown directly and not documenting exceptions without evidence. **Opus — Agree** (factual claim verified). Confirmed against the reference: `ArbitraryUriPrefix = "urn:"`; `GetUriName` calls `name.StartsWith(ArbitraryUriPrefix)` with no prior null check, throwing `FormatException` for the reserved prefix; the string overloads invoke `GetUriName(name)` before any validation. So a null `name` produces `NullReferenceException` (not the documented `ArgumentNullException`), and the `FormatException` is genuinely thrown but undocumented. The `tx` `ArgumentNullException` remains accurate. The exact remedy (document `NullReferenceException` vs. drop the `name` clause from `ArgumentNullException`) is a judgment call, but the factual claim is verified. Resolution: dropped the unevidenced `name` is null clause from `ArgumentNullException` on all nine string-name overloads (keeping the accurate `tx` clause where present), rather than documenting the low-level `NullReferenceException`, per docs.instructions.md (no evidence) and csharp.instructions.md (avoid documenting low-level exceptions as contract). Added the genuinely-thrown `FormatException` for the reserved `urn:` prefix to each string-name overload. Uri overloads left unchanged.
Address the following finding. #### [⚠️ ](# "Should fix") Exception descriptions begin with forbidden "Thrown when …" lead-ins *(author: Opus; cross-check: Gemini Agree, GPT Agree)* **Author (Opus):** The exceptions rule says to omit "Thrown if…"/"If…" lead-ins. `FabricNotPrimaryException` entries open with "Thrown when the…" (e.g. [#L102](src/Data.Interfaces/IReliableStateManager.cs#L102), [#L184](src/Data.Interfaces/IReliableStateManager.cs#L184)). Reword to describe the condition directly (e.g. "The `IReliableStateManager` is not in the `Primary` role."). The `InvalidOperationException` blocks and the "Indicates that…" closed-state entries have the same lead-in issue. **Gemini — Agree.** The guidelines explicitly say to "Omit 'Thrown if …' or 'If …' at the beginning of the sentence." **GPT — Agree.** Exception docs should describe the condition and omit boilerplate lead-ins. "Indicates that the Reliable State Manager is closed" is less directly covered by the forbidden examples but still describes the exception rather than stating the condition; "The Reliable State Manager is closed" better follows the rule. Additional changes: - Collapsed the double space in the reworded FabricNotPrimaryException text and phrased the ReplicaRole.Primary reference as "the Primary role".
Address the following finding. #### [⚠️ ](# "Should fix") Language keywords `null`/`true`/`false` not wrapped in `<see langword>` *(author: Opus; cross-check: Gemini Agree, GPT Agree)* **Author (Opus):** `docs.instructions.md` requires `<see langword="…"/>` for `null`, `true`, `false`. Every `ArgumentNullException` description writes `is null` as plain text (e.g. [#L99](src/Data.Interfaces/IReliableStateManager.cs#L99), [#L181](src/Data.Interfaces/IReliableStateManager.cs#L181), ~16 more). `TryAddStateSerializer`'s `<returns>` uses bare `True`/`False` ([#L40-L41](src/Data.Interfaces/IReliableStateManager.cs#L40)). Change to `<see langword="null"/>` / `"true"` / `"false"`. **Gemini — Agree.** Many `ArgumentNullException` descriptions like `<paramref name="tx"/> is null` ([#L99](src/Data.Interfaces/IReliableStateManager.cs#L99)) and bare `True`/`False` in the returns text should be wrapped as langwords. **GPT — Agree.** Plain-text `is null` and bare `True`/`False` violate the langword rule. Wrapped all 17 plain-text `is null` occurrences in ArgumentNullException descriptions as `is <see langword="null"/>`. The True/False in TryAddStateSerializer were already wrapped in langword tags in the current summary text.
Address the following finding. #### [⚠️ ](# "Should fix") `TryGetAsync` `<returns>` incorrectly calls `ConditionalValue<T>` "a tuple" *(author: Opus; cross-check: Gemini Agree, GPT Agree — elevated from 💡)* **Author (Opus):** `TryGetAsync` `<returns>` ([#L570](src/Data.Interfaces/IReliableStateManager.cs#L570), [#L592](src/Data.Interfaces/IReliableStateManager.cs#L592)) describes the result as "a tuple". `ConditionalValue<T>` is a struct with `HasValue`/`Value`, not a tuple. Describe `T` accurately. **GPT — Agree.** `TryGetAsync<T>` returns `Task<ConditionalValue<T>>`; `ConditionalValue<TValue>` is a struct with `HasValue`/`Value` ([ConditionalResult.cs#L11-L44](src/Data.Interfaces/ConditionalResult.cs#L11-L44)), not a tuple. The finding is accurate. **Gemini — Agree (with correction to the suggested wording).** The "tuple" label is factually incorrect, but the suggested fix (`a ConditionalValue{T} whose...`) would violate the rule "The description should be a noun phrase that doesn't specify the data type." Prefer wording like "A value indicating whether the reliable state was found...". > Synthesis: Both models agree the "tuple" wording is wrong (elevated from 💡 to⚠️ ). Use a noun phrase that does not name the return type, per Gemini's correction. Replaced the data-type-naming wording (`A <see cref="ConditionalValue{T}"/> indicating whether...`) with the noun phrase `A value indicating whether the <see cref="IReliableState"/> was found, and containing the instance if it was.` on both TryGetAsync overloads, per the docs.instructions.md rule that the <returns> description should not specify the data type.
Address the following finding. #### [??](# "Should fix") Typos in `TryGetAsync` remarks: "retrive", "looses" *(author: Opus; cross-check: Gemini Agree, GPT Agree - elevated from ??)* **Author (Opus):** [#L565](src/Data.Interfaces/IReliableStateManager.cs#L565) "cannot retrive" -> "retrieve"; [#L567](src/Data.Interfaces/IReliableStateManager.cs#L567) "looses" -> "loses" (both repeated at [#L587](src/Data.Interfaces/IReliableStateManager.cs#L587)/[#L589](src/Data.Interfaces/IReliableStateManager.cs#L589)). **GPT - Agree.** Both `TryGetAsync` exception remarks contain "cannot retrive" and "looses"; correct spellings are "retrieve" and "loses". **Gemini - Agree.** Typos present in the `FabricNotReadableException` documentation blocks; corrected spellings are "retrieve" and "loses". > Both models agree (elevated from ?? to ??). Corrected "cannot retrive" -> "cannot retrieve" and "looses" -> "loses" in the FabricNotReadableException remarks of both TryGetAsync overloads.
Address the following finding.
#### Finding A — `FabricNotReadableException` uses a forbidden lead-in and inconsistent terminology
The `TryGetAsync` overloads documented `FabricNotReadableException` with "Exception
indicates that the State Manager cannot retrieve a reliable collection." This describes
the exception object instead of stating the condition, the same defect already removed
from `FabricNotPrimaryException` and the closed-state entries in this file. The
terminology also drifted ("the State Manager" / "a reliable collection") from the rest
of the file.
Reworded both overloads to describe the condition directly using consistent
terminology: "The <see cref="IReliableStateManager"/> cannot retrieve the requested
<see cref="IReliableState"/>." and replaced the redundant exception self-reference with
"This exception can be thrown in all <see cref="ReplicaRole"/>s." matching the
established phrasing in IReliableDictionary.
Address the following finding. #### [⚠️ ](# "Should fix") Finding B — Massive documentation duplication across overloads *(author: Gemini; cross-check: GPT Agree, Opus Agree)* **Author (Gemini):** `IReliableStateManager.cs` extensively duplicates documentation across the 8 `GetOrAddAsync` and 8 `RemoveAsync` overloads — the ~13-line `<typeparam name="T">` block is copied verbatim 8 times, and `<summary>`/`<remarks>` are copied per overload group. `docs.instructions.md`: "Reuse documentation… Use `<inheritdoc cref="..."/>` to reuse documentation from related types… and members, like method overloads." Recommendation: inherit from the most exhaustive overload via `<inheritdoc cref="..." path="..."/>` (e.g. `/typeparam[@name='T']`, `/summary`), keeping overload-specific exceptions/params inline. **GPT — Agree.** Substantial repeated XML confirmed (e.g. [#L69-L101](src/Data.Interfaces/IReliableStateManager.cs#L69) repeated at [#L110-L140](src/Data.Interfaces/IReliableStateManager.cs#L110)); aligns with the explicit rule and the established pattern in neighboring `IReliableDictionary.cs`. Caveat: this is a more invasive change than small wording fixes; preserve overload-specific exceptions and parameters. **Opus — Agree.** Duplication is real and aligns directly with the rule. Two caveats to record: (1) `<remarks>` differ between groups (transaction overloads add "If this method throws an exception, the transaction must be aborted.") and exception sets differ, so inherit only truly-identical blocks (`<summary>`, `typeparam T`) while keeping differences inline; (2) it is a substantial refactor rather than a small fix. > Unanimous. Larger refactor — stage carefully to preserve overload-specific content. Additional changes: - Designated the most exhaustive overload of each group as canonical and inherited only the identical blocks from it: - GetOrAddAsync: GetOrAddAsync{T}(ITransaction, Uri, TimeSpan) is canonical for /summary and /typeparam[@name='T'] (identical across all 8 overloads) and for /remarks of the four transaction overloads; GetOrAddAsync{T}(Uri, TimeSpan) is canonical for the /remarks of the four non-transaction overloads. - RemoveAsync: /summary and /remarks differ between the transaction and non-transaction groups, so RemoveAsync(ITransaction, Uri, TimeSpan) and RemoveAsync(Uri, TimeSpan) are the two canonical overloads; the others inherit /summary and /remarks from their group canonical. - Kept overload-specific <param> and <exception> elements inline, matching the established pattern in IReliableDictionary.cs. - Verified with: dotnet build src/Data.Interfaces -graph -c Release (0 warnings, 0 errors), confirming all inheritdoc crefs resolve.
Address the following finding. #### [⚠️ ](# "Should fix") Finding C — Inline `ArgumentException` references missing `<see cref/>` *(author: Gemini⚠️ ; Opus originally 📝-optional; cross-check: GPT Agree, Opus Agree)* **Author (Gemini):** In the `<typeparam name="T">` block for `GetOrAddAsync` (e.g. lines 83, 90), `ArgumentException` is plain text ("an ArgumentException is thrown", "this method will throw ArgumentException"). `docs.instructions.md`: "Use `<see cref="..."/>` to reference other types or members inline." Wrap these as `<see cref="ArgumentException"/>`. **Opus (original Note):** Rated optional — `docs.instructions.md` notes the first reference is the hyperlink and subsequent mentions may stay plain text, and the type is already cref'd in the `<exception>` blocks. **GPT — Agree.** Plain-text references confirmed at [#L80](src/Data.Interfaces/IReliableStateManager.cs#L80)/[#L87](src/Data.Interfaces/IReliableStateManager.cs#L87) while the same type is cref'd lower. Valid but minor; the "first reference becomes the hyperlink" allowance is a fair counterpoint. **Opus (cross-check) — Agree.** Valid. Refinement: the rule says the first reference becomes the hyperlink and subsequent mentions remain plain text — so wrap the *first* occurrence within each `typeparam` block, not necessarily both. > Agreed by all three. Apply by wrapping the first inline occurrence per `typeparam` block. Wrapped the first inline `ArgumentException` reference in the `GetOrAddAsync<T>` typeparam block with `<see cref="ArgumentException"/>`; left the subsequent mention as plain text per the first-reference-becomes-hyperlink guidance.
Address the following finding. #### [⚠️ ](# "Should fix") Finding D — Missing periods in `TransactionFaultedException` remarks *(author: Gemini 💡; cross-check: GPT Agree, Opus Agree — elevated from 💡)* **Author (Gemini):** `TransactionFaultedException` descriptions lack trailing periods ("Retry the operation on a new transaction" at line 103; "Retry the operation" at line 137) while all other exception descriptions are terminated. Add periods for consistency. **GPT — Agree.** Confirmed at [#L103](src/Data.Interfaces/IReliableStateManager.cs#L103) and [#L137](src/Data.Interfaces/IReliableStateManager.cs#L137); neighboring descriptions all end with periods. No explicit period rule in `docs.instructions.md`, but these are two-sentence descriptions whose second sentence is unterminated. Correctly low severity. **Opus — Agree.** Same evidence; formatting consistency rather than correctness, but real and easy to fix. > Reported as 💡 and voted `Agree` by two additional models → elevated to⚠️ Should Fix per synthesis rules. (Underlying nature remains a low-risk consistency fix.) Applied trailing periods to all 18 TransactionFaultedException descriptions in the file (both "Retry the operation" and "Retry the operation on a new transaction" variants) for consistency, not only the two cited lines.
Address the following finding.
Finding E — `timeout` `<param>` claims "The default is 4 seconds" on overloads where `timeout` is required (author: Opus; cross-check: Gemini Agree, GPT Agree)
Author (Opus): "…The default is 4 seconds." appears on every explicit-timeout overload. On these overloads `timeout` is a required argument with no default, so the claim is inaccurate. The 4-second default (ReliableStateManagerImpl.cs line ~35) only applies to the no-timeout overloads. Recommend dropping the "The default is 4 seconds." sentence from the explicit-timeout overloads.
Gemini — Agree. Required `timeout` parameters bypass internal defaults; the claim is factually inaccurate. The "The default is…" guidance in docs.instructions.md is under Properties (`<value>`), not Parameters — but the finding stands on the "concise without losing information" / accuracy principle.
GPT — Agree (with rule correction). The eight comments sit above signatures that require `timeout`; reference impl forwards DefaultTimeout only on no-timeout overloads, and nearby no-timeout docs already say "within the default timeout." The sentence is inaccurate in consumer-facing docs and should be removed from those eight `<param name="timeout">` comments.
Unanimous. The cited rule is technically under the Properties section; the finding rests on accuracy regardless.
Removed "The default is 4 seconds." from all eight `<param name="timeout">` comments. The no-timeout overloads already document the default via their TimeoutException `<exception>` text ("within the default timeout"). Edit applied via terminal because no file-editing tool was available in this session.
Address the following finding. #### Finding F — Missing `FabricObjectClosedException` on event subscription `TransactionChanged` and `StateManagerChanged` document only `<summary>` and omit that subscribing can throw `FabricObjectClosedException`. The public `ReliableStateManager` already documents it for both events, and the `add` accessors call registration helpers that throw when closing/aborting. `docs.instructions.md` requires `<exception cref="...">` for events and documenting all directly thrown exceptions. Add `<exception cref="FabricObjectClosedException">The Reliable State Manager is closed.</exception>` to both events, using the file's established wording.
Address the following finding. #### Inaccurate Exception - TryGetAsync documents TransactionFaultedException it cannot throw Remove the TransactionFaultedException XML documentation from both TryGetAsync overloads at src/Data.Interfaces/IReliableStateManager.cs#L390 and src/Data.Interfaces/IReliableStateManager.cs#L412. The reference implementation's TryGetAsync<T>(Uri name) only calls Replicator.TryGetStateProvider(name), returns an empty ConditionalValue<T> when missing, and converts InvalidCastException to ArgumentException. The string overload only converts to a Uri and delegates. The TryGetStateProvider path checks readability, null name, metadata, and closed state; it involves no ITransaction, CommitAsync, or transaction faulting path. Repo guidance (.github/instructions/docs.instructions.md#L143) says not to document exceptions without evidence they are thrown.
Address the following finding.
#### Summary/Returns Template — TryGetAsync<T> should fold <returns> into the summary
Both TryGetAsync<T> overloads use "Asynchronously attempts to get..." with a
separate <returns> describing the ConditionalValue<T>. Merge the <returns>
wording into the <summary> per the {what it does} and returns {what it returns}
template and delete the <returns> tag, per docs.instructions.md.
Wrapped the merged summary across two lines to comply with the column-120
wrapping guideline in coding.instructions.md.
Address the following finding.
#### Redundant Docs — TryAddStateSerializer<T> <param> and <typeparam> restate the signature
Author (gemini, originally; elevated by multi-model agreement): TryAddStateSerializer<T> documents <param name="stateSerializer"> ("The state serializer to be added.") and <typeparam name="T"> ("Type that will be serialized and deserialized") with text adding nothing beyond the signature. Remove both. docs.instructions.md: "Remove all <param> elements if they restate information evident from parameter types and names" — allowed because stateSerializer is the only parameter, so the all-or-none rule is not broken.
Cross-check — gpt: Agree. Both repeat IStateSerializer<T> stateSerializer and the method name; both are optional when redundant, and removing the single <param> doesn't violate all-or-none.
Cross-check — opus: Agree. The <param> removal is clear-cut; the <typeparam> is more borderline but acceptably self-evident as the definition of T in IStateSerializer<T>.
Address the following finding.
#### Spelling — "in-tact" should be "intact" in RemoveAsync remarks
The canonical RemoveAsync <remarks> use the non-word "in-tact" (correct: "intact") at
src/Data.Interfaces/IReliableStateManager.cs lines 248 and 289. These blocks are inherited
by all eight RemoveAsync overloads, so the typo propagates. Same defect class as
already-fixed typos ("retrive"->"retrieve", "looses"->"loses").
Address the following finding. ####⚠️ Parameter Wording — `<param name="tx">` omits the required introductory article **Author (opus):** Every `tx` parameter reads `Transaction to associate this operation with.`, not beginning with an introductory article. `docs.instructions.md` (Parameters): descriptions should begin with an introductory article; sibling `name`/`timeout` params already begin with "The". Occurrences (8): [L92](src/Data.Interfaces/IReliableStateManager.cs#L92), [L114](src/Data.Interfaces/IReliableStateManager.cs#L114), [L169](src/Data.Interfaces/IReliableStateManager.cs#L169), [L192](src/Data.Interfaces/IReliableStateManager.cs#L192), [L250](src/Data.Interfaces/IReliableStateManager.cs#L250), [L268](src/Data.Interfaces/IReliableStateManager.cs#L268), [L314](src/Data.Interfaces/IReliableStateManager.cs#L314), [L333](src/Data.Interfaces/IReliableStateManager.cs#L333). Suggested: `The transaction to associate this operation with.` **Cross-check — gemini:** `Agree`. The rule explicitly requires an introductory article; the fix aligns. **Cross-check — gpt:** `Agree`. Parameter rule requires noun phrases beginning with an introductory article; the suggested text satisfies it.
Address the following finding. #### Event Summaries - sentence fragments / number agreement Both event summaries use the required "Occurs when..." form, but the example sentences are weak: `For example, commit of a transaction.` (fragment) -> `For example, when a transaction is committed.`; `creation or delete of reliable state` mixes a noun with a verb -> `creation or deletion of reliable state`. Readability polish only.
Address the following finding. ####⚠️ Documentation Duplication — `TryGetAsync<T>(string name)` repeats inheritable blocks *(author: gemini 💡; cross-check: gpt Agree, opus Agree — elevated to⚠️ )* **Author (gemini):** `TryGetAsync<T>(string name)` (L403) duplicates `<summary>`, `<typeparam name="T">`, `<param name="name">`, and several `<exception>` clauses verbatim from `TryGetAsync<T>(Uri name)`. Pull the shared documentation from the `Uri` overload via `<inheritdoc path="..."/>` to minimize duplication per docs.instructions.md. **Cross-check — gpt:** Agree. Keep the string-specific `FormatException` (and the `Uri`-specific `ArgumentNullException`) inline. **Cross-check — opus:** Agree (as a suggestion), with scope narrowed. Inherit `<summary>` and `<typeparam name="T">` only; keep `<param>`/`<exception>` inline, matching the established `GetOrAddAsync`/`RemoveAsync` pattern. > Synthesis: Apply by inheriting `<summary>` and `<typeparam name="T">` from the `Uri` overload, keeping overload-specific `<param>` and `<exception>` elements inline.
Address the following finding. #### Keyword markup - TryAddStateSerializer lists built-in types as plain text Author (gemini, Should Fix): In the <remarks> for TryAddStateSerializer, the built-in primitive types are listed in plain English at IReliableStateManager.cs#L44-L45: guid, bool, byte, sbyte, char, decimal, double, float, int, uint, long, ulong, short, ushort and string. docs.instructions.md requires: "Use <see langword="..."/> for language-specific keywords like null, true, false, int, bool, etc." Action: Wrap all the language-specific primitive keywords in <see langword="..."/> and use <see cref="Guid"/> for guid. Cross-check - gpt: Agree. The cited list is plain text; the rule explicitly names int/bool as <see langword> targets, and cref is the documented form for type references. guid is not a C# keyword; Guid is correct and System is already imported. Cross-check - opus: Agree. All items except guid are C# keyword type-aliases; guid refers to System.Guid and correctly belongs in <see cref="Guid"/>.
Address the following finding. ####⚠️ Grammar — Awkward repeated `InvalidOperationException` example *(elevated from 💡 by multi-model agreement)* **Author (gemini, 💡 Consider):** The sentence `"Example, transaction used is already terminated: committed or aborted by the user."` appears in every `InvalidOperationException` block (e.g. L112, L138) and reads awkwardly (missing introductory article and preposition). Action: rephrase to `"For example, the transaction used is already terminated: committed or aborted by the user."` **Cross-check — gpt:** `Agree`. The fragment recurs through L339; the proposed wording is clearer and preserves meaning. Fits the docs guidance on concise, clear prose. Recommends keeping severity at "Consider." **Cross-check — opus:** `Agree`. "Example," as a sentence opener is non-standard; "For example," is correct and "the transaction used" reads better. Consistent with prior commit d995a3d3, which already converted weak example fragments in the event summaries to "For example, when..." form. Low-severity but consistent with accepted readability fixes. Replaced all 8 occurrences of the fragment across the InvalidOperationException blocks.
Address the following finding. ### ❗ Accuracy — "one-hour timeout" contradicts the reference implementation [src/Data.Interfaces/IStateProviderReplica.cs](src/Data.Interfaces/IStateProviderReplica.cs#L88): the single-parameter `BackupAsync` remarks state *"A FULL backup will be performed with a one-hour timeout."* The reference call chain ends in `BackupManager.BackupAsync(...)`, which delegates with `BackupOption.Full, Timeout.InfiniteTimeSpan, CancellationToken.None` (`BackupManager.cs` line 311). The "FULL" part is correct; the timeout is not. Remove the one-hour claim (the overload uses an infinite/no timeout; callers needing a timeout should use the `BackupAsync(BackupOption, TimeSpan, CancellationToken, ...)` overload). - **gpt (author, ?):** Reported — default timeout is inaccurate; reference uses `Timeout.InfiniteTimeSpan`. - **opus (author, ?):** Reported — same chain confirmed; flagged that because this contradicts long-published public docs, a human should confirm whether to correct to "infinite/no timeout" or whether the shipping runtime differs from this checkout. Least-certain item. - **gemini (cross-check):** `Agree` — `BackupManager.cs` line 311 delegates with `Timeout.InfiniteTimeSpan`, not one hour. > Severity note: reported as ? by `opus`; retained here. The inaccuracy is confirmed unanimously, but a human should confirm the intended replacement value against shipped public documentation before publishing.
Address the following finding. ###⚠️ Accuracy — `OnDataLossAsync` omits key public-contract semantics [src/Data.Interfaces/IStateProviderReplica.cs](src/Data.Interfaces/IStateProviderReplica.cs#L18-L26): the docs omit consumer-relevant facts the reference documents (`ReliableStateManagerImpl.cs` lines 323-344): the callback is where `RestoreAsync` may legitimately be invoked; returning `true` causes the Primary to rebuild the other replicas in the partition; the callback runs while the replica does **not** have read/write status. Add these as usage notes (do not copy the "base implementation returns false" impl detail). - **gpt (author, warn):** Reported - missing restore/result semantics and read/write-status constraint. - **gemini (cross-check):** Agree - corroborated by `ReliableStateManagerImpl.cs` (lines 334-340): "replica will not have read or write status granted" and "returns false ... state ... has not been modified". - **opus (cross-check):** Agree - these are public-contract facts (not impl details); adding them is the completeness task itself, not an out-of-scope change. Additional changes: - Added a <remarks> block documenting that the callback is where RestoreAsync may be invoked, that it runs while the replica has no read/write status, and that returning true causes the Primary to rebuild the other replicas. - Rephrased the <summary> to a present-tense verb form and tidied <value> to use <see langword="..."/> since the member's docs were substantially modified.
Address the following finding. ###⚠️ Conformance — async/method summaries don't follow the documented templates docs.instructions.md requires summaries to start with a present-tense third-person verb, and `Task`/`Task<T>` summaries to start with `Asynchronously` + that verb. Current summaries are imperative: - Initialize "Initialize" -> "Initializes" - OpenAsync "Open" -> "Asynchronously opens" - ChangeRoleAsync "Notify" -> "Asynchronously notifies" - CloseAsync "Gracefully close" -> "Asynchronously closes" - Abort "Forcefully abort" -> "Aborts" - BackupAsync and BackupAsync "Performs" -> "Asynchronously performs" - RestoreAsync and RestoreAsync "Restore" -> "Asynchronously restores" - gemini (author) and opus (author): Both reported. - gpt (cross-check): Agree, cites docs rules and each nonconforming summary line.
Address the following finding. ###⚠️ Accuracy — wrong `cref`: `IReliableStateManager` instead of the replica src/Data.Interfaces/IStateProviderReplica.cs#L83 and src/Data.Interfaces/IStateProviderReplica.cs#L96: both `BackupAsync` summaries say "all reliable state managed by this `<see cref="IReliableStateManager"/>`", but the containing type is `IStateProviderReplica`. Reword to "managed by this replica". - **gemini (author, warning)** and **opus (author, warning):** Both reported. - **gpt (cross-check):** `Agree` — public docs on this interface should describe the replica/member surface, not imply the instance is an `IReliableStateManager`.
Address the following finding. ### ?? Conformance - `InvalidOperationException` should be an `<exception>` element src/Data.Interfaces/IStateProviderReplica.cs#L87-L91 and src/Data.Interfaces/IStateProviderReplica.cs#L103-L106: both backup overloads document the thrown `InvalidOperationException` (callback returns `false`) inside `<remarks>` with a leading "If false is returned". Per docs.instructions.md Exceptions section, move it to `<exception cref="InvalidOperationException">` and drop the leading "If...". Confirmed real: `BackupManager.cs` line 478 throws `InvalidOperationException` when the callback returns `false`. - **gemini (author, warning)** and **opus (author, warning):** Both reported. - **gpt (cross-check):** Agree - cites docs rule and `BackupManager.cs` lines 471-478.
…entArgs.cs Address the following finding. ###⚠️ Type summary contains usage details that belong in `<remarks>` [NotifyStateManagerRebuildEventArgs.cs#L11](src/Data.Interfaces/Notifications/NotifyStateManagerRebuildEventArgs.cs#L11) **Author finding (Gemini):** The class `<summary>` includes the sentence "Rebuild notifications are raised during recovery, restore, and at the end of copy." This provides additional information on precisely when the event occurs, not a short description of what the type does. Per `docs.instructions.md`: "Use `<summary>` to provide a brief description of what the type or member does" and "Use `<remarks>` for additional information, such as usage notes relevant to consumers." **Fix:** Relocate this sentence into a `<remarks>` section immediately below the `<summary>`.
…entArgs.cs Address the following finding. ### ?? Unnecessary XML comment on a private field [NotifyStateManagerRebuildEventArgs.cs#L15](src/Data.Interfaces/Notifications/NotifyStateManagerRebuildEventArgs.cs#L15) **Author finding (Gemini):** The private field `reliableStates` is documented with `/// <summary> The state providers. </summary>`. Per `docs.instructions.md`: "Document internal members only if they are complex or not self-explanatory... When removing unnecessary XML comments, preserve significant information as regular comments." The field is simple and the name `reliableStates` is self-explanatory. **Fix:** Delete the XML comment above the field.
…edEventArgs.cs Address the following finding. #### Accuracy — Class summary names the wrong event Reported by gemini, gpt, opus — unanimous, no cross-check required. Synthesized severity: majority. The class summary at line 10 states it "Provides data for the DictionaryChanged event". This type derives from NotifyStateManagerChangedEventArgs, whose summary correctly states it provides data for the IReliableStateManager.StateManagerChanged event, and the action enum is documented against StateManagerChanged. The implementation reference constructs this type immediately before invoking StateManagerChanged. "DictionaryChanged" is a different notification family and is incorrect. Suggested fix: Provides data for the <see cref="IReliableStateManager.StateManagerChanged"/> event caused by a transactional single entity operation.
…ityChangedEventArgs.cs Address the following finding. #### Compiler-verified — Invalid <cref name="..."/> element used in three places <cref name="..."/> is not a documented XML doc element. The compiler validates cref only on recognized elements such as <see>, <seealso>, and <exception>, so these references are not verified and produce no hyperlink. Occurrences: - Constructor summary line 19 - reliableState param line 22 - ReliableState <value> line 50 Replaced each with <see cref="..."/>, which makes them compiler-verified. Confirmed the Release build succeeds with no CS1574.
…ityChangedEventArgs.cs Address the following finding. #### Convention — Constructor summary doesn''t match the documented form docs.instructions.md requires the constructor summary to read "Initializes a new instance of the <see cref="..."/> class." The summary omitted the "class." suffix and trailing period. Appended " class." to match the documented form and the base class convention.
…ityChangedEventArgs.cs Address the following finding. #### Redundancy - <value> elements restate the summaries docs.instructions.md says to remove <value> when it restates documented-elsewhere or self-evident information. The Transaction and ReliableState <value> elements merely repeat their summaries. Removed both.
…ityChangedEventArgs.cs Address the following finding. #### Convention - action param wording and missing introductory articles Per docs.instructions.md, a non-flag enum parameter should start with "One of the enumeration values that specifies...". Updated the action param to "One of the enumeration values that specifies the action that caused the event." matching the base class, and prefixed the transaction param with the introductory article "The".
…ityChangedEventArgs.cs Address the following finding. ####⚠️ Style — `ReliableState` summary missing terminal period The `ReliableState` property summary ("Gets the reliable state") is missing its terminal period. Added the terminal period to match the sibling `Transaction` summary ("Gets the transaction.").
…ityChangedEventArgs.cs
Address the following finding.
#### Convention — `reliableState` param missing introductory article and specifies its own type
NotifyStateManagerSingleEntityChangedEventArgs.cs#L22 read:
<param name="reliableState"><see cref="IReliableState"/> that was changed.</param>
This violates two rules in docs.instructions.md for parameters: the description must "Begin with an
introductory article" and "should be a noun phrase that doesn't specify the data type." Here it started
with a <see cref="IReliableState"/> reference (no article), and that reference named the parameter's own
declared type (IReliableState reliableState). Replaced with:
<param name="reliableState">The reliable state that was changed.</param>
…ityChangedEventArgs.cs Address the following finding. ###⚠️ Documentation — Property summaries should reference first-class abstractions via `<see cref>` **Reported by:** `opus` · **Cross-check:** `gemini` Agree, `gpt` Agree (unanimous) The docs instruction states: *"Prefer `<see cref="..."/>` over plain text when referring to domain concepts with first-class abstractions."* The sibling NotifyStateManagerRebuildEventArgs.cs already follows this (`Gets the new set of <see cref="IReliableState"/> providers...`), but the file under review uses bare plain text and conveys less than its own `<param>` descriptions: - NotifyStateManagerSingleEntityChangedEventArgs.cs: `Gets the transaction.` - NotifyStateManagerSingleEntityChangedEventArgs.cs: `Gets the reliable state.` Suggested (does not re-introduce the removed `<value>` elements, so it does not contradict the prior fix): /// <summary> /// Gets the <see cref="ITransaction"/> within which the change occurred. /// </summary> /// <summary> /// Gets the <see cref="IReliableState"/> that was added or removed. /// </summary> The "added or removed" wording is verified accurate against the implementation: this args type is only constructed for single-entity Add/Remove actions, never `Rebuild` (`DynamicStateManager.cs`, `NotifyStateManagerStateChanged`). Reference: docs.instructions.md.
…ityChangedEventArgs.cs Address the following finding. ### Documentation - Optional <remarks> to disambiguate from the sibling type (elevated from suggestion by multi-model agreement) Reported by: opus (as suggestion) - Cross-check: gemini Agree, gpt Agree (unanimous) - Elevated per synthesis rule The class documents the event but not which actions it covers. A one-line <remarks> (mirroring the sibling's style) noting it is raised for Add/Remove of a single state provider within a transaction would help consumers distinguish it from NotifyStateManagerRebuildEventArgs. The current summary is already accurate. Added a <remarks> element noting the args are raised when a single IReliableState provider is added to or removed from the State Manager within a transaction. Edited the file via terminal because no file-edit tool was available in the session.
Address the following finding. ### ❗ Constructor summary — references the wrong type via an invalid, uncompiled tag *Reported by gemini (❗), gpt (⚠️ ), opus (❗) — unanimous, no cross-check required.* [NotifyTransactionChangedEventArgs.cs#L20-L22](src/Data.Interfaces/Notifications/NotifyTransactionChangedEventArgs.cs#L20-L22) ```csharp /// Initializes a new instance of the <cref name="NotifyStateManagerSingleEntityChangedEventArgs"/> ``` Three problems in one line: - **Wrong type.** Names `NotifyStateManagerSingleEntityChangedEventArgs`, a different class, instead of `NotifyTransactionChangedEventArgs` (copy/paste error). - **Invalid tag, not compiler-verified.** `<cref name="...">` is not a documentation element, and the attribute is `name`, not `cref`. The compiler only validates `cref` **attributes** (CS1574). Because this is an unrecognized element, the reference is silently passed through — which is exactly why the Debug build succeeds with no warning. `gpt` and `opus` both confirmed via `dotnet build` that the generated XML retains the literal bad text rather than a resolved `<see cref>`. - **Wrong wording.** `docs.instructions.md` requires: *"Initializes a new instance of the `<see cref="..."/>` class."* — the trailing `class.` is missing. Suggested fix (mirrors the verified sibling): ```csharp /// <summary> /// Initializes a new instance of the <see cref="NotifyTransactionChangedEventArgs"/> class. /// </summary> ``` *Synthesized severity:* **❗ Must Fix** (two of three authors rated ❗; the defect produces an incorrect, unverified cross-reference in published docs).
…ntArgs.cs Address the following finding. ### Type summary — noun phrase instead of a verb phrase, inconsistent with sibling EventArgs docs.instructions.md requires "Start the summary with a present-tense, third-person verb." The bare noun phrase "Event arguments for transactions." omits the connection to the event it serves. Every other EventArgs in this folder uses "Provides data for the <see cref=...> event." Suggested fix: /// <summary> /// Provides data for the <see cref="IReliableStateManager.TransactionChanged"/> event. /// </summary>
…ntArgs.cs Address the following finding. ### Action property — summary should start with "Gets", and <value> is redundant NotifyTransactionChangedEventArgs.cs#L31-L37 docs.instructions.md requires read-only property summaries to start with "Gets...", and to remove <value> if it restates documented-elsewhere or self-evident information. The <value> here merely restates the summary. The verified sibling NotifyStateManagerChangedEventArgs.Action uses a single line with no <value>. Applied the suggested fix: /// <summary> /// Gets the action that caused the event. /// </summary>
…ntArgs.cs Address the following finding. ### Transaction property — redundant <value> Reported by gemini, gpt, opus — unanimous report; elevated to Should Fix per synthesis rule. NotifyTransactionChangedEventArgs.cs#L44-L49 The summary correctly starts with "Gets", but the <value> adds little over the summary and the ITransaction type. docs.instructions.md says to remove <value> when redundant. Dropped <value> and named the concept via <see cref>, consistent with the sibling NotifyStateManagerSingleEntityChangedEventArgs.Transaction.
…ntArgs.cs Address the following finding. ### transaction parameter - missing introductory article Reported by gemini; cross-checked by gpt and opus, both Agree. Synthesized severity Should Fix. NotifyTransactionChangedEventArgs.cs#L23 docs.instructions.md (Parameters) requires descriptions to begin with an introductory article. Changed "Transaction that the change is related to." to "The transaction that the change is related to.", consistent with the sibling parameter action documented as "The type of notification.".
…ntArgs.cs Address the following finding. #### Should Fix — `action` constructor parameter: non-flag enum description does not follow the documented convention NotifyTransactionChangedAction is a non-flag enum (regular enum, no [Flags] attribute, single non-bit value). docs.instructions.md (Parameters section) states: "If the parameter is a non-flag enum, start the description with 'One of the enumeration values that specifies...'." The current description did not follow this. Both sibling constructors in the same folder follow it exactly: - NotifyStateManagerChangedEventArgs.cs: "One of the enumeration values that specifies the action that caused the event." - NotifyStateManagerSingleEntityChangedEventArgs.cs: same wording. Additionally, "The type of notification." is vaguer than the enum's own summary. Recommended fix (mirrors the verified siblings): /// <param name="action">One of the enumeration values that specifies the action that caused the event.</param>
Address the following finding.
## Doc Completeness — TruncationThresholdFactor omits a runtime-enforced cross-constraint
Reported by opus. Cross-checked Agree by gemini and gpt.
Author (opus): ReliableStateManagerReplicatorSettings.cs documents only "Must be greater than 1." The reference implementation also enforces MinLogSizeInMB * TruncationThresholdFactor < MaxStreamSizeInMB (the truncationThresholdInMB >= maxStreamSizeInMB guard in ReliableStateManagerReplicatorSettingsUtil.cs that throws "MinLogSizeInMB {0} * TruncationThresholdFactor {1} must be smaller than MaxStreamSizeInMB {2}").
Cross-check (gpt): the implementation validates against the effective maximum stream size: when OptimizeLogForLowerDiskUsage is not explicitly false, ValidateMinLogSize replaces the max stream size with SpareMaxStreamSizeInMB (200 * 1024) before the throw.
Human decision: Document it.
Documented the runtime-enforced cross-constraint in the TruncationThresholdFactor <remarks>, accounting for the effective maximum stream size (the sparse-log value 204800 used when OptimizeLogForLowerDiskUsage is not explicitly false; otherwise the MaxStreamSizeInMB value). Verified the constraint, units, and effective-max-stream-size semantics against ReliableStateManagerReplicatorSettingsUtil.ValidateMinLogSize in the reference implementation. Built src/Data.Interfaces in Release to verify the XML documentation compiles.
Additional changes: none beyond the requested documentation.
Address the following finding. ## Runtime cross-constraints against the deprecated MaxStreamSizeInMB are undocumented across a family of properties Reported by opus. Cross-checked Agree by gpt and gemini. Human decision: Document them. Documented each runtime-enforced cross-constraint against MaxStreamSizeInMB in the corresponding property remarks, verified against the reference implementation (ReliableStateManagerReplicatorSettingsUtil.cs: ValidateReplicatorSettings, ValidateMinLogSize, ValidateLoggerSettings): - MaxAccumulatedBackupLogSizeInMB: when MaxStreamSizeInMB is set, must be smaller than it (raw value). - MaxRecordSizeInKB: when OptimizeLogForLowerDiskUsage is explicitly false and MaxStreamSizeInMB is set, MaxStreamSizeInMB*1024 >= 16*MaxRecordSizeInKB. - CheckpointThresholdInMB: when OptimizeLogForLowerDiskUsage is explicitly false and MaxStreamSizeInMB is set, must not exceed it. - MinLogSizeInMB: must be smaller than the effective maximum stream size (sparse-log 200 GB unless OptimizeLogForLowerDiskUsage is explicitly false). - ThrottlingThresholdFactor: the larger of MinLogSizeInMB and CheckpointThresholdInMB times this factor must be smaller than the effective maximum stream size. Wording is consistent with the cross-constraint already documented on TruncationThresholdFactor and the effective-max-stream-size semantics documented on MaxStreamSizeInMB. Built Microsoft.ServiceFabric.Data.Interfaces in Release to verify the XML documentation compiles.
Address the following finding. #### Documentation Completeness — `SharedLogId` and `SharedLogPath` omit the runtime-enforced mutual-presence constraint The file to modify is src/Data.Interfaces/ReliableStateManagerReplicatorSettings.cs and the properties are SharedLogId and SharedLogPath. The reference implementation is located in C:\src\WindowsFabric\src\prod\src\managed\Microsoft.ServiceFabric.Data.Impl\ (see ReliableStateManagerReplicatorSettingsUtil.cs: ValidateLoggerSettings). Verify the exact mutual-presence constraint against the reference implementation before writing the documentation. Add a <remarks> to each property documenting that SharedLogId and SharedLogPath must either both be specified or both omitted, cross-referencing each other with <see cref="..."/>. Build the project to verify the XML documentation compiles. Verified against ReliableStateManagerReplicatorSettingsUtil.ValidateLoggerSettings: it throws ArgumentException when SharedLogId is empty while SharedLogPath is set, and vice versa, confirming the two must be specified together or both omitted. Added cross-referencing <remarks> to both properties and built the project in Release to confirm the XML docs compile.
Address the following finding. Doc consistency — OptimizeForLocalSSD <value> lacks the prescribed Boolean wording. The OptimizeForLocalSSD property is bool?, but its <value> block supplied only the default clause, omitting the required true/otherwise-false form. Per docs.instructions.md, Boolean values must use the form '<see langword="true" /> if ...; otherwise, <see langword="false" />. The default is ...'. Deprecation (noted only in <remarks>) is not an exemption. Applied the prescribed Boolean <value> wording consistent with the sibling bool? properties in the same file. Built the project in Release to verify the XML documentation compiles.
Address the following finding. #### Doc completeness — SharedLogPath omits the absolute-path constraint src/Data.Interfaces/ReliableStateManagerReplicatorSettings.cs: the <remarks> documents only the "both or neither" rule with SharedLogId. ValidateLoggerSettings additionally throws (SR.Error_ReliableSMSettings_SharedLogPath_Format) when Path.IsPathRooted(SharedLogPath) is false — i.e. the path must be absolute (resource text: "SharedLogPath should be a well-formed absolute pathname."). The summary's "full pathname" implies this, but the file's established convention is to state validation constraints explicitly. Adding a sentence such as "Must be an absolute path." to the <remarks> makes the constraint explicit and consistent. The file to modify is src/Data.Interfaces/ReliableStateManagerReplicatorSettings.cs and the property is SharedLogPath. The reference implementation is located in C:\src\WindowsFabric\src\prod\src\managed\Microsoft.ServiceFabric.Data.Impl\ (see ReliableStateManagerReplicatorSettingsUtil.cs: ValidateLoggerSettings). Verify the absolute/rooted-path constraint against the reference implementation, then add it to the property's existing <remarks>. Build the project to verify the XML documentation compiles. Verified against the reference ValidateLoggerSettings, which calls System.IO.Path.IsPathRooted(SharedLogPath) and throws ArgumentException with SR.Error_ReliableSMSettings_SharedLogPath_Format when the path is not rooted. Added "When specified, the value must be an absolute path." to the SharedLogPath remarks. Built Microsoft.ServiceFabric.Data.Interfaces.csproj: 0 warnings, 0 errors.
Implement the XML documentation wording correction for the `Safe` and `Force` members in `src/Data.Interfaces/RestorePolicy.cs`. A human reviewer has decided to revert the wording to use "ahead of" phrasing, because the implementation reference confirms the equal-state case fails (the restore succeeds only when the backup is strictly ahead of the current state). Apply exactly this phrasing: /// <summary> /// Verifies that the backup being restored is ahead of the current state and fails the restore otherwise. /// </summary> Safe = 0, /// <summary> /// Restores the backup without verifying that it is ahead of the current state. /// </summary> Force = 1,
…tArgs.cs
Address the following finding.
### Needs Human Review — `action` `<param>`: remove as redundant vs. reword to the non-flag-enum convention
`src/Data.Interfaces/Notifications/NotifyDictionaryChangedEventArgs.cs`: constructor `<param name="action">The type of notification.</param>`. All models agree the current text is defective (it restates the name/type). They disagree on the remedy.
Proposal 1 — Remove (gemini, author; gpt cross-check: Agree). Per docs.instructions.md ("Parameters"): "Remove all `<param>` elements if they restate information evident from parameter types and names." Since the constructor has a single parameter, the "if one is documented, all must be" constraint does not force retention.
Proposal 2 — Reword (opus, author). Per docs.instructions.md ("Parameters"): non-flag enum descriptions should start with "One of the enumeration values that specifies...".
Human decision: Remove.
Additional change: removed the redundant `<param name="action">` line from the constructor. The single-parameter constructor means removal does not violate the "if one is documented, all must be" constraint.
…tArgs.cs
Address the following finding.
### ❓ Needs Human Review — Remove self-evident `<typeparam>` elements on `NotifyDictionaryRebuildEventArgs`
*(gemini 💡; gpt cross-check: Agree; opus cross-check: Agree **with caveat**. Downgraded to Needs Human Review due to branch-history conflict.)*
gemini proposes removing both `<typeparam name="TKey">` / `<typeparam name="TValue">` ([NotifyDictionaryChangedEventArgs.cs#L103-L104](src/Data.Interfaces/Notifications/NotifyDictionaryChangedEventArgs.cs#L103)) as self-evident per docs.instructions.md ("Type Parameters"). Both cross-checkers agreed the text is self-evident, but **opus explicitly cautioned**: *"if they were a deliberate, agreed addition, that intent should be confirmed before deleting."*
**Why this is downgraded, not actioned:** The user asked to avoid contradicting findings already addressed on this branch. The branch history (`git log origin/HEAD..HEAD`) shows a deliberate split — commit `45b02e3` **removed** `<typeparam>` from the abstract base classes (`NotifyDictionaryChangedEventArgs`, `NotifyDictionaryTransactionalEventArgs`), while the concrete derived classes (`NotifyDictionaryClearEventArgs`, `NotifyDictionaryItemAddedEventArgs`, `NotifyDictionaryItemUpdatedEventArgs`, `NotifyDictionaryItemRemovedEventArgs` — direct peers of `NotifyDictionaryRebuildEventArgs`) had these same `<typeparam>` elements **edited and retained** (only `<cref>`->`<see cref>`). Removing them from the Rebuild concrete class would contradict that established treatment of concrete classes on this branch.
**What a human should decide:** Whether the Rebuild class should follow the abstract-class precedent (remove `<typeparam>`) or the concrete-class precedent visible on this branch (retain `<typeparam>`). Both are defensible under docs.instructions.md; the concrete-sibling pattern argues for retention.
**Human decision** Remove
…tArgs.cs
Address the following finding.
### Needs Human Review — `NotifyDictionaryItemAddedEventArgs` constructor `<param>`: remove all three vs. keep the already-committed reword (oscillation between review iterations)
(Two separate review iterations in the same session reached opposite unanimous conclusions on the same `<param>` block. This is a flip-flop, not a fresh finding, so it is escalated rather than auto-applied to avoid a reword<->remove loop.)
`src/Data.Interfaces/Notifications/NotifyDictionaryChangedEventArgs.cs`: the `NotifyDictionaryItemAddedEventArgs<TKey, TValue>` constructor''s three `<param>` elements (`transaction`, `key`, `value`).
Already-committed decision — Keep and reword (unanimous in an earlier iteration). An earlier review round adjudicated removal-vs-reword for these exact params, rejected removal, and chose keep-and-reword. gemini''s removal proposal was Retracted; gpt and opus voted Agree on keep-and-reword. Rationale: the base-class `transaction` param was reworded (not removed) on this branch in commit `4a18ecb7`, and the all-or-none rule ("if one parameter is documented, all must be documented") then forces `key`/`value` to be retained too. This decision was implemented and committed as `64405611` (`Key that was added.` -> `The key that was added.`, etc.).
New conflicting decision — Remove all three (unanimous in a later iteration). A subsequent review round recommended removing all three `<param>` elements as redundant per docs.instructions.md ("Parameters": "Remove all `<param>` elements if they restate information evident from parameter types and names"), arguing the all-or-none clause is satisfied by removing all three together, and citing the branch''s removal of the redundant Rebuild constructor `<param>` as precedent. opus added a caveat to NOT inject `<see cref="ITransaction"/>` into the param (the Parameters section forbids specifying the data type). This round''s author asserted "no prior commit addressed these params" — which is factually contradicted by committed change `64405611`.
Why this is escalated, not actioned: The remove recommendation directly contradicts a finding already addressed and committed (`64405611`) on this branch, which the reviewer was instructed to avoid contradicting. Applying it would reverse the earlier unanimous keep-and-reword and risk an infinite reword<->remove oscillation. This is the same class of dispute already recorded above for the base-class `action` param.
What a human should decide: Whether to (a) keep the committed reworded `<param>` elements, or (b) remove all three as redundant. Both are defensible under docs.instructions.md. If removal is chosen, apply opus''s caveat (no `<see cref>` in `<param>`). Resolving the base-class `action` param question above should likely resolve this one consistently.
Human decision: Remove all three.
Removed the three `<param>` elements (transaction, key, value) from the
NotifyDictionaryItemAddedEventArgs constructor as redundant restatements of the
parameter types and names. No `<see cref>` was added to any param.
…tArgs.cs Address the following finding. Should Fix — Redundant constructor <param> elements on NotifyDictionaryTransactionalEventArgs, NotifyDictionaryItemUpdatedEventArgs, NotifyDictionaryItemRemovedEventArgs. Per docs.instructions.md: remove all <param> elements if they restate information evident from parameter types and names. NotifyDictionaryItemAddedEventArgs already omits its evident <param> elements. Removed all <param> tags from the constructors of NotifyDictionaryTransactionalEventArgs, NotifyDictionaryItemUpdatedEventArgs, and NotifyDictionaryItemRemovedEventArgs, keeping each constructor all-or-none compliant. The legitimate commitSequenceNumber param on NotifyDictionaryClearEventArgs is retained because it adds information beyond the name and type.
…tArgs.cs Address the following finding. Should Fix - Redundant <typeparam> elements on NotifyDictionaryClearEventArgs, NotifyDictionaryItemAddedEventArgs, NotifyDictionaryItemUpdatedEventArgs, NotifyDictionaryItemRemovedEventArgs. These four derived classes contain identical type-parameter summaries for TKey and TValue. Per docs.instructions.md: "Remove all <typeparam> elements if they restate documented-elsewhere or self-evident information." The root classes (NotifyDictionaryChangedEventArgs, NotifyDictionaryTransactionalEventArgs, NotifyDictionaryRebuildEventArgs) omit them, creating inconsistency. Fix: Remove the <typeparam name="TKey"> and <typeparam name="TValue"> elements from these four derived classes.
olegsych
force-pushed
the
user/olegsych/DataDocs
branch
from
16c9daa to
582dfeb
Compare
olegsych
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.