18 AL/BC patterns: style, data-modeling, web-services, appsource, breaking-changes, performance, testing - #156
Conversation
…ices, appsource, breaking-changes, performance, and testing Contributed by CURABIS ApS, generalized from patterns observed across real AppSource/PTE development. Each article follows the knowledge file format (frontmatter, Description/Best Practice/Anti Pattern, sibling .good.al/.bad.al samples).
Jesper Schulz-Wedde (JesperSchulz)
left a comment
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
There are useful topics here, but this is not safe to merge as agent guidance yet.
The repository validator reports six R14 errors because three article stems do not match their companion sample stems (do-not-change-primary-key-field-list, testfield-required-setup-field-before-use, and al-identifiers-must-be-english). Beyond that structural failure, several normative claims and fixtures conflict with the current platform/API: Email Message is a codeunit, the dimension sample calls a nonexistent DimensionManagement.ValidateShortcutDimCode API, cloud-scoped File calls fail compilation rather than compiling and being silently skipped, and the proposed AL build setting is not an AL Language setting.
The selection contract also needs updating. None of the leaf review skills changes in this PR, and several new articles cannot enter their worklists from the currently enumerated tokens/cues (for example file-datatype-saas, prefer-email-module, namespace-must-be-verified-from-source, and var-parameters-require-an-addressable-variable). The skills currently claim their targeted checks cover every current article, so please add deterministic cues/tokens for the new rules rather than relying on fuzzy topic matching.
I have left inline details on the confirmed factual and fixture issues. The knowledge-index and neutral-fixture validators pass, but frontmatter/sample validation fails. The CLA check is also still pending and must be resolved separately before merge.
|
@microsoft-github-policy-service agree company="CURABIS ApS" |
- Rename 3 articles so their .good.al/.bad.al companion stems match (do-not-change-primary-key, testfield-required-setup-field, al-identifiers-english), fixing the R14 orphan-sample errors. - do-not-change-primary-key.good.al: include Flow in the new table's own primary key so it actually models the discriminating dimension. - al-build-output-must-not-pollute-project-root.md: drop the unsubstantiated AL0197 causal claim and the non-existent al.outputPath setting; reframe as build-artifact hygiene sourced from ALTool --outfolder / al_build outputPath. - prefer-email-module.md: Email Message is Codeunit 8904, not a table; distinguish it from the underlying Sent/Outbox/Draft storage. - file-datatype-saas.md: File.Open/Create/Read/Write fails to compile against a Cloud-scoped project, it does not compile and silently fail at runtime. - namespace-must-be-verified-from-source.md: narrow to "resolve from the referenced object's source or symbols," since source-file line one is not the only authoritative source (symbol packages, comments before the namespace line). - test-data-must-be-random-and-complete.md: drop "assume an empty database" and "collision-free" absolutes; reframe around independence from unrelated business records and reserving explicit values for scenario-defining inputs. - binary-choice-must-be-boolean.md: scope to genuine true/false semantics, not mechanical two-member-enum-to-boolean conversion. - document-report-word-layout.md: scope down to a sourced Microsoft Learn recommendation instead of an unconditional performance guarantee; cite the three Learn pages. - Wire the new articles into their review skills' candidate-selection signals (file-datatype-saas, prefer-email-module, namespace-must-be-verified-from-source, var-parameters-require-an- addressable-variable) so they can actually enter a worklist. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…eDim do not exist on the current DimensionManagement codeunit Verified against microsoft/BCApps: the real master-table validation procedure is ValidateDimValueCode (or ValidateShortcutDimValues when a DimSetID is also needed), and the real document-side inheritance procedure is GetDefaultDimID, not CreateDim. Caught from Jesper Schulz-Wedde's review thread, which had been partially hidden by GitHub's comment folding. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Jesper Schulz-Wedde (JesperSchulz)
left a comment
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
Follow-up after the latest two commits: many of the original issues are fixed, including fixture discovery, unsupported settings guidance, the Email Message object type, cloud File behavior, the replacement-table key, Boolean semantics, and Word/RDL guidance. The remaining correctness issues are inline below.
One integration issue also remains: please add deterministic candidate-selection cues for the new data-modeling, testing, performance, and web-services rules whose actual anti-patterns are not already discoverable by their owning review skills. Passing the content validators does not by itself make those rules reachable by an agent review.
|
|
||
| ## Description | ||
|
|
||
| Adding dimension support to a custom master or document table is not just a matter of adding a `Code[20]` field. Business Central expects a specific set of hooks into `Codeunit "Dimension Management"` so a dimension value is validated, persisted as a Default Dimension record, and flows through to transactions the same way it does for every standard table. Skipping any one hook produces a field that looks correct in the designer but silently fails to save, validate, or carry through to postings. |
There was a problem hiding this comment.
This still conflates two different dimension models. Master data persists Default Dimension records, while transactional records carry a Dimension Set ID assembled from inherited/default dimensions. A master record does not generally acquire a DimSetID through SaveDefaultDim, and a document needs more than adding shortcut fields. Please split these cases and make the good fixture implement every hook the prose requires, including inheritance into the document's dimension set.
There was a problem hiding this comment.
Fixed in 584143b — split the article into the two distinct models: master data (Default Dimension via ValidateDimValueCode/SaveDefaultDim, no Dimension Set ID field) versus transactional/document data (a single Dimension Set ID built from AddDimSource + GetDefaultDimID, inheriting the master's Default Dimension records, with ValidateShortcutDimValues updating that same ID rather than writing a separate Default Dimension record). Added a second, document-table example (Course Registration Header) alongside the existing master-table one, using the real 3-arg AddDimSource(var DefaultDimSource, TableId, No) and 6-arg GetDefaultDimID(DefaultDimSource, SourceCode, var GlobalDim1Code, var GlobalDim2Code, InheritFromDimSetID, InheritFromTableNo) signatures — verified against DimensionManagement.Codeunit.al and the real call site in ExchRateAdjmtProcess.Codeunit.al.
There was a problem hiding this comment.
Thanks—the prose now separates Default Dimensions from transactional Dimension Set IDs, which addresses the conceptual part of this thread. Two defects remain in the new good fixture:
SaveDefaultDim(..., FieldNo("Global Dimension 1 Code"), ...)passes table field ID 10, but that parameter is the shortcut-dimension number1..8; this example should pass1.- On customer change,
GetDefaultDimIDreceives the document's existing Dimension Set ID with inherited table number0. That can preserve values inherited from the previous customer instead of recomputing the new customer's defaults. Please model this after a current documentCreateDimimplementation (normally recomputing source defaults without treating the existing document set as an anonymous inherited source), or explicitly define and test how user overrides are meant to win.
Once those fixture behaviors are corrected, I think this conversation can close.
|
|
||
| ## Description | ||
|
|
||
| Every journal-based posting routine in Business Central is split across three companion codeunits with distinct, non-overlapping responsibilities: `Check Line` validates one line, `Post Line` writes exactly one line to the ledger, and `Post Batch` loops both across the journal. A document posting routine (posting one document at a time) calls `Post Line` directly and skips `Post Batch`. A new posting routine that blurs this split either misses functionality other code expects to call directly, or exposes an interaction surface it shouldn't. |
There was a problem hiding this comment.
This turns a useful standard architecture into a universal invariant. Not every journal-based posting implementation has exactly these three companions or these non-overlapping responsibilities, and document posting does not universally call the same Post Line directly. Please present this as a pattern to evaluate in context, identify the standard implementations it is based on, and replace the sample's nonexistent "Meter Jnl.-Check Line" dependency so the good fixture is internally complete.
There was a problem hiding this comment.
Fixed in 584143b — reframed from a universal invariant to "the standard shape to evaluate against," naming the real codeunits it's based on (Gen./Item/Resource/Job/FA/Insurance/CA Jnl.-Check Line/-Post Line/-Post Batch — all verified to exist under those names). Also added the missing Check Line companion codeunit to the good fixture so it's internally complete.
There was a problem hiding this comment.
Thanks—the contextual caveat and the added Check Line fixture resolve part of the concern. The article still describes the responsibilities as “distinct, non-overlapping” and says Post Line writes “exactly one line to the ledger.” The standard General Journal implementation contradicts both claims: Gen. Jnl.-Post Line.RunWithCheck calls Gen. Jnl.-Check Line.RunCheck, and posting one journal line can create multiple G/L, VAT, balancing, rounding, and deferral entries.
Please describe the split as processing scope and reusable entry points instead: Check Line validates one journal line; Post Line processes/posts one journal line, may reuse validation, and may create multiple ledger entries; Post Batch orchestrates the batch.
|
|
||
| ## Anti Pattern | ||
|
|
||
| Writing a `using` statement from memory, from an incomplete path, or from a plausible-looking guess. It can compile in one build environment while still failing to resolve in VS Code, because the two use different namespace resolution. |
There was a problem hiding this comment.
The source/symbol guidance is now sound, but this explanation is not. The compiler and AL language server do not intentionally use different namespace-resolution rules; differences usually come from stale/different symbols, configuration, or build context. Please describe those concrete causes rather than teaching agents that the two resolvers inherently disagree.
There was a problem hiding this comment.
Fixed in 584143b — removed the claim that the compiler and AL Language Server apply different namespace-resolution rules; the Anti Pattern section now attributes a wrong resolution to stale/mismatched symbols, a different build configuration, or the object's actual current source instead.
There was a problem hiding this comment.
The Markdown explanation is corrected, thanks. The companion bad fixture still says // resolves in a local build, fails in VS Code without showing stale/different symbols or configuration, so it continues to teach the inherent resolver discrepancy that the article now rejects. Please either make the guessed namespace fail against the same symbols in both environments, or explicitly model the differing symbol/configuration context. Then this thread is resolved.
- dimension-management-wiring.md/.good.al: split into the two distinct models the article was conflating - master data (Default Dimension records via ValidateDimValueCode/SaveDefaultDim) vs. transactional/ document data (a single Dimension Set ID assembled via AddDimSource + GetDefaultDimID, verified against BCApps' ExchRateAdjmtProcess.Codeunit.al). Added a compiling document-table example alongside the existing master table one. - Deleted api-page-flowfields-must-be-calcfields (.md/.good.al/.bad.al): Microsoft's own FlowFields documentation states a FlowField used as a control's direct source expression is automatically calculated on any page - no API-page exception is documented, and none could be reproduced. - prefer-email-module.bad.al/.md: Codeunit Mail has no Send/GetErrorDesc members; fixed to the real current 7-argument CreateMessage signature, and corrected the claim that the legacy path "still runs" - its base implementation no longer sends anything, only raises integration events. - check-post-line-batch-pattern.md/.good.al: reframed from a universal invariant to the standard shape, naming the real Gen./Item/CA/Res./Job/ Insurance/Mfg. Item/FA Jnl.-Check Line/-Post Line/-Post Batch codeunits it's based on. Added the missing Check Line companion codeunit so the good fixture is internally complete. - test-data-must-be-random-and-complete.good.al: removed leftover "collision-free" wording contradicting the already-corrected article text. - fixed-choice-set-must-use-enum-not-integer.md: removed the reintroduced state-count heuristic ("the line is the state count"), aligned with binary-choice-must-be-boolean.md's semantics-based distinction. - namespace-must-be-verified-from-source.md: removed the false claim that the compiler and AL Language Server use different namespace-resolution rules. - intrinsic-al-functions-must-use-modern-casing.md: removed the unverified claim that PascalCase is the VS Code formatter's default output. Worklist completeness: added cues for the 8 rules in data-modeling, testing, performance, and web-services that had none (Jesper's explicit ask), plus the same gap in all 7 style rules from this PR (not explicitly named this round, but the identical systemic issue) - 15 cues total across al-data-modeling-review.md, al-testing-review.md, al-performance-review.md, al-web-services-review.md, and al-style-review.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Jesper Schulz-Wedde (@JesperSchulz) Addressed this round in 584143b (pushed 2026-09-08): the 7 file-level issues above (individual replies inline), plus the worklist-completeness ask — added deterministic cues for the 8 rules you named without any (data-modeling/testing/performance/web-services) and, since I found the identical gap, for all 7 style rules from this PR too (15 cues total, across al-data-modeling-review.md, al-testing-review.md, al-performance-review.md, al-web-services-review.md, al-style-review.md). Ready for another look whenever you have time. |
2 participants
Summary
18 knowledge articles generalized from patterns observed across real AppSource/PTE Business Central development, contributed by CURABIS ApS. Each is a self-contained article following the knowledge file format (6-key frontmatter, Description/Best Practice/Anti Pattern, sibling
.good.al/.bad.alsamples, no fenced code in the .md).Targeted at existing Microsoft-owned domains per the README's contribution guidance ("use
/microsoft/knowledge/for Microsoft-owned domains") rather than/community/, since these all fall under domains BCQuality already owns action skills for.Checked against the current
microsoft/knowledge/corpus before opening this PR (post the recent community→microsoft consolidation) — two originally-drafted articles were dropped as duplicates of existing files (xrec-is-a-before-image-only-in-some-triggers.md,use-library-codeunits-for-test-fixtures.md+asserterror-needs-expectederror-and-code.md).Test plan
🤖 Generated with Claude Code