Skip to content

Improve summary documentation#23036

Merged
tyler-cai-microsoft merged 8 commits into
microsoft:mainfrom
tyler-cai-microsoft:add-more-summary-documentation
Nov 12, 2024
Merged

Improve summary documentation#23036
tyler-cai-microsoft merged 8 commits into
microsoft:mainfrom
tyler-cai-microsoft:add-more-summary-documentation

Conversation

@tyler-cai-microsoft
Copy link
Copy Markdown
Contributor

@tyler-cai-microsoft tyler-cai-microsoft commented Nov 8, 2024

AB#15222

Improve summarizer documentation

  • SummaryTreeBuilder documentation
  • Other summary cleanup

@tyler-cai-microsoft tyler-cai-microsoft changed the title Add more summary documentation Improve summary documentation Nov 8, 2024
@github-actions github-actions Bot added area: runtime Runtime related issues base: main PRs targeted against main branch labels Nov 8, 2024
@tyler-cai-microsoft tyler-cai-microsoft requested a review from a team as a code owner November 8, 2024 19:18
@github-actions github-actions Bot added the public api change Changes to a public API label Nov 8, 2024
Copy link
Copy Markdown
Collaborator

@msfluid-bot msfluid-bot left a comment

Choose a reason for hiding this comment

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

Code Coverage Summary

↓ packages.runtime.runtime-utils.src:
Line Coverage Change: 0.04%    Branch Coverage Change: -0.06%
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 93.93% 93.87% ↓ -0.06%
Line Coverage 97.83% 97.87% ↑ 0.04%

Baseline commit: c7730cc
Baseline build: 306006
Happy Coding!!

Code coverage comparison check passed!!

@msfluid-bot
Copy link
Copy Markdown
Collaborator

msfluid-bot commented Nov 8, 2024

@fluid-example/bundle-size-tests: +245 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 464.33 KB 464.36 KB +35 Bytes
azureClient.js 563.18 KB 563.23 KB +49 Bytes
connectionState.js 724 Bytes 724 Bytes No change
containerRuntime.js 262.48 KB 262.5 KB +14 Bytes
fluidFramework.js 426.84 KB 426.85 KB +14 Bytes
loader.js 134.18 KB 134.19 KB +14 Bytes
map.js 42.71 KB 42.71 KB +7 Bytes
matrix.js 148.36 KB 148.36 KB +7 Bytes
odspClient.js 528.97 KB 529.02 KB +49 Bytes
odspDriver.js 97.88 KB 97.9 KB +21 Bytes
odspPrefetchSnapshot.js 42.81 KB 42.83 KB +14 Bytes
sharedString.js 164.26 KB 164.27 KB +7 Bytes
sharedTree.js 417.3 KB 417.3 KB +7 Bytes
Total Size 3.37 MB 3.37 MB +245 Bytes

Baseline commit: c7730cc

Generated by 🚫 dangerJS against e27d50f

Comment thread packages/runtime/runtime-utils/src/summaryUtils.ts Outdated
tyler-cai-microsoft and others added 3 commits November 8, 2024 13:10
Co-authored-by: Craig Macomber (Microsoft) <42876482+CraigMacomber@users.noreply.github.com>
}

/**
* Adds an attachment to the summary. This blob needs to already be uploaded to storage.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nothing in this doc comment or signature explains why "blob" is mentioned here. Or what an attachment is. Or how you get its id.

Linking to ISummaryAttachment or even
Linking to ISummaryAttachment.id would probably help. Also if this has nothing to do with ISummaryBlob, avoiding the term blob would prabably be good.

Also given that ISummaryAttachment.id is undocumented, adding a doc to it and linking it would be good.

Note that ISummaryAttachment.id is read/write, so someone not knowing better could try and set the id to what ever they want, which I assume wouldn't work. Making its docs clear about that would be good (maybe fix the field to be readonly: its kinda late for that so if changes it would gets its own PR and go through API review).

Also seems odd that when adding blobs and handles you provide the key under which the blob will be stored, but when adding an attachment, it puts in under a key it generates itself in an undocumented way.

It seems like a really odd choice for this method to pick the key for you, and the rest to require the key.

If I want to store some attachment as part of my summary, how would the loading code read it back from the summary without knowing the key? This seems hard to use and inconsistent with the other APIs. If there is a reason it works this way, the docs should probably note it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Regardless of these issues, this PR is a clear improvement, so I'll approve.

Copy link
Copy Markdown
Contributor Author

@tyler-cai-microsoft tyler-cai-microsoft Nov 12, 2024

Choose a reason for hiding this comment

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

Also seems odd that when adding blobs and handles you provide the key under which the blob will be stored, but when adding an attachment, it puts in under a key it generates itself in an undocumented way.

I don't know enough historical context to document this part, so I'm going to leave it as is here.


// TODO type I can infer from SummaryObject. File mode I may want to directly specify so have symlink+exec access
/**
* The object containing all the tree's {@link SummaryObject} children.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we expand on what the path keys are? Any invariants for those keys?

Comment thread packages/runtime/runtime-utils/src/summaryUtils.ts Outdated
}

/**
* Use this once you're done building the summary tree, the stats should automatically be generated.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: I would recommend rephrasing this in terms of what it does, rather than when it should be called. Those details could be added in a @remarks block if they are useful, but the core part of the comment should be in terms of the method's semantics.

Copy link
Copy Markdown
Contributor

@Josmithr Josmithr left a comment

Choose a reason for hiding this comment

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

+1 to Craig's comment. Left some suggestions, but this is a clear improvement! Thank you very much for adding documentation!!!

tyler-cai-microsoft and others added 3 commits November 12, 2024 11:50
@tyler-cai-microsoft tyler-cai-microsoft enabled auto-merge (squash) November 12, 2024 19:55
@github-actions
Copy link
Copy Markdown
Contributor

🔗 Found some broken links! 💔

Run a link check locally to find them. See
https://github.com/microsoft/FluidFramework/wiki/Checking-for-broken-links-in-the-documentation for more information.

linkcheck output


> fluidframework-docs@0.25.0 ci:linkcheck /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test ci:start 1313 linkcheck:full

1: starting server using command "npm run ci:start"
and when url "[ 'http://127.0.0.1:1313' ]" is responding with HTTP status code 200
running tests using command "npm run linkcheck:full"


> fluidframework-docs@0.25.0 ci:start
> http-server ./public --port 1313 --silent


> fluidframework-docs@0.25.0 linkcheck:full
> npm run linkcheck:fast -- --external


> fluidframework-docs@0.25.0 linkcheck:fast
> linkcheck http://localhost:1313 --skip-file skipped-urls.txt --external

Crawling...

http://localhost:1313/community/
- (124:32) 'Microsof..' => https://opensource.microsoft.com/codeofconduct/ (connection failed)


Stats:
  443722 links
    3414 destination URLs
       2 URLs ignored
       1 warnings
       1 errors

 ELIFECYCLE  Command failed with exit code 1.

@tyler-cai-microsoft tyler-cai-microsoft merged commit c699aee into microsoft:main Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: runtime Runtime related issues base: main PRs targeted against main branch public api change Changes to a public API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants