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.

Sign up for GitHub

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

Jump to bottom

refactor(api-markdown-documenter): Update structure of ApiTransformationConfiguration and ApiItemTransformations #23381

Merged
merged 4 commits into from
Dec 20, 2024
Merged

refactor(api-markdown-documenter): Update structure of ApiTransformationConfiguration and ApiItemTransformations #23381

merged 4 commits into from
Dec 20, 2024

Conversation

Josmithr
Copy link
Contributor

Updated the structure of ApiTransformationConfiguration to contain a transformations property of type ApiItemTransformations, rather than implementing that interface directly.

Also updates ApiItemTransformations methods to be keyed off of ApiItemKind, rather than being individually named.

E.g. A call like config.transformApiMethod(...) would become config.transformations["Method"](...).

This better aligns with similar transformational API surfaces in this library, like the renderers. It also makes it easier to ensure we capture all possible ApiItem kinds.

The createDefaultLayout property of ApiItemTransformations now lives directly in ApiTransformationConfiguration, but has been renamed to defaultSectionLayout.

@Josmithr Josmithr requested review from alexvy86, a team and Copilot December 20, 2024 01:05
@github-actions github-actions bot added public api change Changes to a public API base: main PRs targeted against main branch labels Dec 20, 2024
Copilot
Copilot AI reviewed Dec 20, 2024
View reviewed changes

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 17 changed files in this pull request and generated no comments.

Files not reviewed (12)
  • tools/api-markdown-documenter/src/api-item-transforms/default-implementations/TransformApiInterface.ts: Evaluated as low risk
  • tools/api-markdown-documenter/src/api-item-transforms/configuration/Transformations.ts: Evaluated as low risk
  • tools/api-markdown-documenter/src/api-item-transforms/default-implementations/CreateSectionForApiItem.ts: Evaluated as low risk
  • tools/api-markdown-documenter/src/api-item-transforms/default-implementations/TransformApiClass.ts: Evaluated as low risk
  • tools/api-markdown-documenter/src/api-item-transforms/TransformApiModel.ts: Evaluated as low risk
  • tools/api-markdown-documenter/src/api-item-transforms/configuration/Configuration.ts: Evaluated as low risk
  • tools/api-markdown-documenter/src/api-item-transforms/default-implementations/TransformApiEnum.ts: Evaluated as low risk
  • tools/api-markdown-documenter/src/api-item-transforms/default-implementations/TransformApiFunctionLike.ts: Evaluated as low risk
  • tools/api-markdown-documenter/src/api-item-transforms/default-implementations/index.ts: Evaluated as low risk
  • tools/api-markdown-documenter/src/api-item-transforms/default-implementations/TransformApiItemWithoutChildren.ts: Evaluated as low risk
  • tools/api-markdown-documenter/src/api-item-transforms/test/Transformation.test.ts: Evaluated as low risk
  • tools/api-markdown-documenter/src/api-item-transforms/default-implementations/TransformApiModuleLike.ts: Evaluated as low risk
Josmithr
Josmithr commented Dec 20, 2024
View reviewed changes
tools/api-markdown-documenter/src/api-item-transforms/configuration/Configuration.ts
*
* @returns The list of {@link SectionNode}s that comprise the top-level section body for the API item.
*/
readonly defaultSectionLayout: (
Copy link
Contributor Author

@Josmithr Josmithr Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Extracted from ApiItemTransformations, since it really isn't a "transformation"; it's a utility used by transformations. It will also make it easier to convert ApiItemTransformations to a mapped type (over ApiItemKind) in the future.

Josmithr
Josmithr commented Dec 20, 2024
View reviewed changes
tools/api-markdown-documenter/src/api-item-transforms/configuration/Configuration.ts
/**
* {@inheritDoc ApiItemTransformationConfiguration.defaultSectionLayout}
*/
readonly defaultSectionLayout?: (
Copy link
Contributor Author

@Josmithr Josmithr Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I plan to deduplicate this with the duplicate signature above in a future change. For now, this is simpler given the mix of properties that are required in both "configuration" and "options", and the properties that are optional in "options".

Josmithr
Josmithr commented Dec 20, 2024
View reviewed changes
tools/api-markdown-documenter/src/api-item-transforms/TransformApiItem.ts Outdated

logger.verbose(`Rendering section for ${apiItem.displayName}...`);
logger.verbose(`Generating documentation section for ${apiItem.displayName}...`);

let sections: SectionNode[];
switch (apiItem.kind) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: the exhaustive switch statement is still required since each transformation is strongly typed over its ApiItem implementation. Hoping to simplify this in the future.

alexvy86
alexvy86 approved these changes Dec 20, 2024
View reviewed changes
Copy link
Contributor

@alexvy86 alexvy86 left a comment

Choose a reason for hiding this comment

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

A couple small things, otherwise lgtm!

tools/api-markdown-documenter/src/api-item-transforms/configuration/Transformations.ts Outdated Show resolved Hide resolved
tools/api-markdown-documenter/src/api-item-transforms/configuration/Transformations.ts
Comment on lines -80 to -82
/**
* Transformation to generate a {@link SectionNode} for a `Call Signature`.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we keep all the existing docs? I see only a few were kept.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept the couple that had special notes on them. For the others, they seemed redundant. Cases like this always give me pause - I generally prefer docs even in cases like this, but the amount of spatial clutter they create in the file is a bit annoying :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'm going to leave this alone for now. Once this becomes a mapped type, and some of the special cases have been normalized, this all becomes a lot cleaner.

@github-actions GitHub Actions
Copy link
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> fluid-framework-docs-site@0.0.0 ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-docs-site@0.0.0 serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> fluid-framework-docs-site@0.0.0 check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  170138 links
    1596 destination URLs
    1825 URLs ignored
       0 warnings
       0 errors

@Josmithr Josmithr merged commit cfe75ab into microsoft:main Dec 20, 2024
30 checks passed
@Josmithr Josmithr deleted the api-markdown-documenter/transformations-property branch December 20, 2024 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@alexvy86 alexvy86 alexvy86 approved these changes

Assignees
No one assigned
Labels
base: main PRs targeted against main branch public api change Changes to a public API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Josmithr @alexvy86