mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-06 09:26:52 +01:00
Add P0 event and interface compatibility knowledge (#98)
* Add P0 extensibility compatibility knowledge Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 645349fd-1892-48f3-8a84-db77d6abd1c3 * Correct event compatibility guidance Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 645349fd-1892-48f3-8a84-db77d6abd1c3 --------- Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
This commit is contained in:
parent
078b869e33
commit
363f08f47e
19 changed files with 331 additions and 32 deletions
|
|
@ -39,7 +39,7 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
|
||||
- The changed AL object names and types — especially codeunits that publish events or host event subscribers, posting/release/validation routines that should expose extension points, and test codeunits that bind subscribers.
|
||||
- The changed procedures and triggers, weighted toward event publisher methods, methods carrying the `[EventSubscriber(...)]` attribute, routines that raise `OnBefore`/`OnAfter` events, and any procedure that calls `BindSubscription`/`UnbindSubscription`.
|
||||
- Tokens extracted from the diff that relate to events and the publish/subscribe model (`IntegrationEvent`, `BusinessEvent`, `EventSubscriber`, `IsHandled`, `BindSubscription`, `UnbindSubscription`, `EventSubscriberInstance`, `OnBefore`, `OnAfter`, `Manual`, `IncludeSender`, `Sender`, `this`, `RecordRef`, `xRec`, `temporary`, `Temp`, `repeat`).
|
||||
- Tokens extracted from the diff that relate to events and the publish/subscribe model (`IntegrationEvent`, `BusinessEvent`, `InternalEvent`, `EventSubscriber`, `IsHandled`, `BindSubscription`, `UnbindSubscription`, `EventSubscriberInstance`, `OnBefore`, `OnAfter`, `Manual`, `IncludeSender`, `GlobalVarAccess`, `Isolated`, `local`, `internal`, `Sender`, `this`, `RecordRef`, `xRec`, `temporary`, `Temp`, `repeat`).
|
||||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.
|
||||
|
||||
|
|
@ -53,13 +53,15 @@ The following targeted checks map diff signals to specific `events` articles. Tr
|
|||
|
||||
- `IsHandled` raised without an immediately preceding `IsHandled := false;`, or one `IsHandled` variable reused across several raises with no reset between them — `initialize-ishandled-to-false-before-publishing`.
|
||||
- `if IsHandled then exit;` in a routine that also raises a paired `OnAfter…` event later, so the after-event is skipped whenever the call is handled — `preserve-onafter-execution-when-ishandled-skips-the-body`.
|
||||
- A parameter added before existing parameters on a changed event signature instead of appended at the end — `add-new-event-parameters-at-the-end`.
|
||||
- Any parameter added to a public Business/Integration event procedure, regardless of position; do not flag additions or reordering on `local`/`internal` publishers merely because a new parameter was not appended — `add-new-event-parameters-at-the-end`.
|
||||
- A shipped Business/Integration event renamed or removed, or an existing parameter renamed, removed, retyped, or changed to/from `var`, based on the mistaken assumption that `local` or `internal` prevents dependent subscription; parameter order alone is not a subscriber-contract violation — `treat-local-and-internal-events-as-subscriber-contracts`.
|
||||
- Any change to `IncludeSender` or `GlobalVarAccess` on a shipped event at any target version, or to `Isolated` on BC20/runtime 9.0 or later, including a change intended to modernize the publisher — `do-not-change-shipped-event-attribute-flags`.
|
||||
- Publisher names that do not encode firing position (`OnBefore`/`OnAfter<Routine>` at the boundaries, `On<Routine>OnBefore`/`OnAfter<Context>` mid-routine) — `name-events-by-publisher-position`.
|
||||
- Two consecutive `OnBefore`/`OnAfter` raises with no logic between them, or a near-duplicate event differing only by an extra parameter — `prefer-reusing-or-extending-existing-events`.
|
||||
- An event raised between `repeat` and `until` inside a record loop — `do-not-publish-events-inside-loops`.
|
||||
- A `temporary` record event parameter whose name does not start with `Temp` — `prefix-temporary-record-event-parameters-with-temp`.
|
||||
- Abbreviated event parameter names (`SalesHdr`, `DocNo`, `Amt`) instead of full table names and spelled-out values — `name-event-parameters-without-abbreviations`.
|
||||
- `[IntegrationEvent(true, …)]` (`IncludeSender`) on a codeunit event used only to expose the publisher, where `this` could be passed as a typed `Sender` parameter (Business Central 2024 release wave 2 and later) — `prefer-this-over-includesender-in-codeunit-events`.
|
||||
- `[IntegrationEvent(true, …)]` (`IncludeSender`) on a newly added codeunit event used only to expose the publisher, where `this` could be passed as a typed `Sender` parameter (Business Central 2024 release wave 2 and later) — `prefer-this-over-includesender-in-codeunit-events`.
|
||||
- A `RecordRef` event parameter, or a passed-through `xRec`, where a concrete typed record fits — `avoid-loosely-typed-event-parameters`.
|
||||
- A `var IsHandled` added to a pre-existing event rather than introduced through a new `OnBefore` publisher — `do-not-add-ishandled-to-an-existing-event`.
|
||||
- An `if IsHandled then exit;` whose skipped body performs posting, ledger-entry creation, number-series consumption, or integrity/permission validation — `do-not-bypass-critical-operations-with-ishandled`.
|
||||
|
|
|
|||
|
|
@ -39,7 +39,7 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
|
||||
- The changed AL object names and types — especially `interface` objects, codeunits and enums declared with the `implements` keyword, and consumers that declare or assign an `Interface` variable.
|
||||
- The changed procedures and triggers, weighted toward factory or dispatch routines that resolve a variant to behaviour, setter-injection procedures that take an `Interface` parameter, and `case`-over-enum blocks that select between strategies.
|
||||
- Tokens extracted from the diff that relate to interfaces and enum-backed implementation (`interface`, `implements`, `Implementation`, `DefaultImplementation`, `UnknownValueImplementation`, `enum`, `Extensible`, `Interface`, `case`, and the `case <enum> of` anti-pattern signal — a `case` over an enum value whose branches choose between variant computations).
|
||||
- Tokens extracted from the diff that relate to interfaces and enum-backed implementation (`interface`, `extends`, `implements`, `Implementation`, `DefaultImplementation`, `UnknownValueImplementation`, `enum`, `Extensible`, `Interface`, `case`, and the `case <enum> of` anti-pattern signal — a `case` over an enum value whose branches choose between variant computations).
|
||||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.
|
||||
|
||||
|
|
@ -47,6 +47,14 @@ Once the candidate worklist is known, resolve layer-precedence conflicts per REA
|
|||
|
||||
When the post-conflict worklist is empty because no applicable interfaces knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable interfaces knowledge matched the changes, emit `outcome: "completed"` with an empty `findings` array.
|
||||
|
||||
### Interface-compatibility checks
|
||||
|
||||
The following targeted checks map diff signals to specific `interfaces` articles. Treat each as a candidate-selection cue: when the signal appears in the changed code, add the named article to the worklist and evaluate it in Action.
|
||||
|
||||
- `DefaultImplementation` used as the only fallback where a persisted ordinal may no longer match any declared enum value, or a persisted enum lacks `UnknownValueImplementation` on BC18 or later — `handle-unknown-enum-ordinals-with-unknownvalueimplementation`.
|
||||
- A method added directly to an interface that exists in the baseline, instead of adding a BC25+ interface that `extends` it or a versioned sibling for older targets — `extend-published-interfaces-dont-edit-them`.
|
||||
- A declared enum value with no `Implementation` and no enum-level `DefaultImplementation` — `set-defaultimplementation-on-enum`.
|
||||
|
||||
## Action
|
||||
|
||||
For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Emit findings as follows:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue