Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(generate:assertTags): Enable per package assert tagging configuration #23452

Merged
merged 23 commits into from
Jan 21, 2025
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
aadc2e0
Setup tagging for tree's "fail"
CraigMacomber Jan 3, 2025
c16f987
Fix fail tagging
CraigMacomber Jan 3, 2025
5e1ce1f
remove fail templates
CraigMacomber Jan 3, 2025
89ce236
Update fail to allow tagged messages
CraigMacomber Jan 3, 2025
3d5fee3
Merge branch 'main' into failTag
CraigMacomber Jan 7, 2025
5c06f20
Work on comments
CraigMacomber Jan 7, 2025
e55dd41
fix error aggregation and extend comment
CraigMacomber Jan 7, 2025
a67474c
Apply suggestions from code review
CraigMacomber Jan 7, 2025
2c8fe5c
workaround assert tagging to make fail call assert
CraigMacomber Jan 7, 2025
137a6c2
Fix newAssetFiles typo
CraigMacomber Jan 7, 2025
472cf03
Merge branch 'main' of https://github.com/microsoft/FluidFramework in…
CraigMacomber Jan 8, 2025
fbac2c2
Merge branch 'main' of https://github.com/microsoft/FluidFramework in…
CraigMacomber Jan 8, 2025
18ae12c
Give assert tagging its own config
CraigMacomber Jan 8, 2025
c400e3c
Merge branch 'main' of https://github.com/microsoft/FluidFramework in…
CraigMacomber Jan 9, 2025
46cfaa3
remove old config
CraigMacomber Jan 9, 2025
23cc67d
Merge branch 'main' into failTag
CraigMacomber Jan 13, 2025
5a7b3dc
Document config
CraigMacomber Jan 14, 2025
b2fca3d
Document configs
CraigMacomber Jan 14, 2025
9593d26
Update generated docs
CraigMacomber Jan 14, 2025
24efa74
Merge branch 'main' into failTag
CraigMacomber Jan 16, 2025
01d79dc
Merge branch 'main' of https://github.com/microsoft/FluidFramework in…
CraigMacomber Jan 16, 2025
240b8ab
Restrict config to only config.mjs
CraigMacomber Jan 16, 2025
3d42046
Merge branch 'main' into failTag
CraigMacomber Jan 21, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions build-tools/packages/build-cli/api-report/build-cli.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,16 @@ import { InterdependencyRange } from '@fluid-tools/version-tools';
import { run } from '@oclif/core';
import { VersionBumpType } from '@fluid-tools/version-tools';

// @public (undocumented)
// @public
export interface AssertTaggingConfig {
// (undocumented)
enabledPaths?: RegExp[];
}

// @public
export interface AssertTaggingPackageConfig {
assertionFunctions: {
[functionName: string]: number;
};
enabledPaths?: RegExp[];
}

// @public
Expand Down
267 changes: 181 additions & 86 deletions build-tools/packages/build-cli/src/commands/generate/assertTags.ts

Large diffs are not rendered by default.

13 changes: 11 additions & 2 deletions build-tools/packages/build-cli/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ export interface FlubConfig {

/**
* Configuration for assert tagging.
* @remarks
* Some of this applies to the root where flub is being run,
* and some of it applies to the specific package being processed.
* @privateRemarks
* It seems like having each package have its own configuration would be simpler.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could make it work that way for this command. fluid-build supports task definitions in both places, but flub has not historically needed such a capability outside of the typetest commands. I'm inclined to leave handling individual package config to individual commands for now.

@zhenmichael Something to keep an eye on as you port commands to the new infrastructure. Are there more commands that could benefit from a general-purpose per-package config loading capability? Maybe that feature could be baked into the new PackageCommand base class from the beginning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are going to look at general things we want for configuration, I think the big one thats missing is any sort of schema/validation system.

Using a library like typebox to give you errors if you have a typo in your config field names or invalid structure would be nice (instead of just casting config data parsed as json to a type an hoping its right). Would have saved me some annoyance when I messed up the nesting working on this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using typescript types in the JS config files helps prevent a lot of these errors. You're absolutely right about the stuff in package.json.

*/
assertTagging?: AssertTaggingConfig;

Expand Down Expand Up @@ -212,12 +217,16 @@ export interface PolicyConfig {
publicPackageRequirements?: PackageRequirements;
}

/**
* Used by `TagAssertsCommand`.
*/
export interface AssertTaggingConfig {
assertionFunctions: { [functionName: string]: number };

/**
* An array of paths under which assert tagging applies to. If this setting is provided, only packages whose paths
* match the regular expressions in this setting will be assert-tagged.
*
* This is used from the root where flub is run.
* TODO: this should be replaced by package selection flags passed to the command.
*/
enabledPaths?: RegExp[];
}
Expand Down
3 changes: 3 additions & 0 deletions build-tools/packages/build-cli/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,6 @@ export type {
ScriptRequirement,
} from "./config.js";
export type { knownReleaseGroups, ReleaseGroup, ReleasePackage } from "./releaseGroups.js";

// Exported for use in config files.
export type { AssertTaggingPackageConfig } from "./commands/generate/assertTags.js";
3 changes: 0 additions & 3 deletions fluidBuild.config.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -585,9 +585,6 @@ module.exports = {

assertTagging: {
enabledPaths: [/^common\/lib\/common-utils/i, /^experimental/i, /^packages/i],
assertionFunctions: {
assert: 1,
},
},

// `flub bump` config. These settings influence `flub bump` behavior for a release group. These settings can be
Expand Down
9 changes: 9 additions & 0 deletions packages/dds/test-dds-utils/assertTagging.config.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/*!
* Copyright (c) Microsoft Corporation and contributors. All rights reserved.
* Licensed under the MIT License.
*/

/**
* @type {import("@fluid-tools/build-cli").AssertTaggingConfig}
*/
export default { assertionFunctions: {} };
CraigMacomber marked this conversation as resolved.
Show resolved Hide resolved
14 changes: 14 additions & 0 deletions packages/dds/tree/assertTagging.config.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/*!
* Copyright (c) Microsoft Corporation and contributors. All rights reserved.
* Licensed under the MIT License.
*/

/**
* @type {import("@fluid-tools/build-cli").AssertTaggingConfig}
*/
export default {
assertionFunctions: {
"assert": 1,
"fail": 0,
},
};
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ export function makeV1Codec(
context,
);
case NoopMarkType:
fail(`Mark type: ${type} should not be encoded.`);
fail("Mark type: NoopMarkType should not be encoded.");
default:
unreachableCase(type);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ export function makeV2CodecHelpers(
context,
);
case NoopMarkType:
fail(`Mark type: ${type} should not be encoded.`);
fail("Mark type: NoopMarkType should not be encoded.");
default:
unreachableCase(type);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/dds/tree/src/simple-tree/api/treeNodeApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ export const treeNodeApi: TreeNodeApi = {
changedFields,
(field) =>
nodeSchema.storedKeyToPropertyKey.get(field) ??
fail(`Could not find stored key '${field}' in schema.`),
fail("Could not find stored key in schema."),
),
);
listener({ changedProperties });
Expand Down
2 changes: 1 addition & 1 deletion packages/dds/tree/src/simple-tree/toMapTree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ function nodeDataToMapTree(
result = objectToMapTree(data, schema);
break;
default:
fail(`Unrecognized schema kind: ${schema.kind}.`);
fail("Unrecognized schema kind");
}

return result;
Expand Down
13 changes: 11 additions & 2 deletions packages/dds/tree/src/util/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,17 @@ export function asMutable<T>(readonly: T): Mutable<T> {

export const clone = structuredClone;

export function fail(message: string): never {
throw new Error(message);
/**
* Throw an error with a constant message.
* @remarks
* Works like {@link @fluidframework/core-utils/internal#assert}.
*/
export function fail(message: string | number): never {
// Declaring this here aliased to a different name avoids the assert tagging objecting to the usages of `assert` below.
// Since users of `fail` do the assert message tagging instead, suppressing tagging errors here makes sense.
const assertNoTag: (condition: boolean, message: string | number) => asserts condition =
assert;
assertNoTag(false, message);
}

/**
Expand Down
9 changes: 9 additions & 0 deletions packages/runtime/test-runtime-utils/assertTagging.config.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/*!
* Copyright (c) Microsoft Corporation and contributors. All rights reserved.
* Licensed under the MIT License.
*/

/**
* @type {import("@fluid-tools/build-cli").AssertTaggingConfig}
*/
export default { assertionFunctions: { assertionFunctions: {} } };
9 changes: 9 additions & 0 deletions packages/test/assertTagging.config.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/*!
* Copyright (c) Microsoft Corporation and contributors. All rights reserved.
* Licensed under the MIT License.
*/

/**
* @type {import("@fluid-tools/build-cli").AssertTaggingConfig}
*/
export default { assertionFunctions: {} };
Loading