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

fix(docs): Don't inherit @system notices from ancestry #23312

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

Josmithr
Copy link
Contributor

@Josmithr Josmithr commented Dec 12, 2024

It is valid for a non-@system API to extend a @system one. See here for more details on our tags, including @system. In fact, this is a common pattern - we introduce a base interface that some of our API members extend. We don't want customers to use that base interface directly, but it needs to exist for one reason or another. In this case, we don't want the implementations of that interface to inherit the @system notice.

AB#22762

@github-actions github-actions bot added area: website base: main PRs targeted against main branch labels Dec 12, 2024
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:
  170098 links
    1596 destination URLs
    1825 URLs ignored
       0 warnings
       0 errors


@Josmithr Josmithr requested a review from a team December 12, 2024 18:22
@Josmithr Josmithr marked this pull request as ready for review December 12, 2024 18:22
@Josmithr Josmithr changed the title improvement(docs): Don't inherit @system notices from ancestry fix(docs): Don't inherit @system notices from ancestry Dec 12, 2024

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no suggestions.

Comments skipped due to low confidence (1)

docs/infra/api-markdown-documenter/render-api-documentation.mjs:95

  • The change from ancestryHasModifierTag to hasModifierTag may cause the function to miss @system tags inherited from parent items. Ensure this change aligns with the intended behavior of not inheriting @system notices from ancestry.
if (ApiItemUtilities.hasModifierTag(apiItem, "@system")) {

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

@@ -92,7 +92,7 @@ export async function renderApiDocumentation(inputDir, outputDir, uriRootDir, ap
createDefaultLayout: layoutContent,
getAlertsForItem: (apiItem) => {
const alerts = [];
if (ApiItemUtilities.ancestryHasModifierTag(apiItem, "@system")) {
if (ApiItemUtilities.hasModifierTag(apiItem, "@system")) {
alerts.push("System");
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about using constants for these tag names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally, I think the code reads more easily with them as inline literals, especially since they're each only used in one place AFAIK. I don't feel too strongly about this, but I think I will at least consider this beyond the scope of the PR.

@Josmithr Josmithr merged commit f7e9c40 into microsoft:main Dec 13, 2024
42 checks passed
@Josmithr Josmithr deleted the docs/system-tag-cleanup branch December 13, 2024 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: website base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants