docs(js-sdk): 3 docstrings name parameters the functions do not take - #1740
docs(js-sdk): 3 docstrings name parameters the functions do not take#1740karpovantonme wants to merge 1 commit into
Conversation
Volume.makeDir and Volume.writeFile each document 2 options parameters, options and opts, while the signature takes 1: opts?: VolumeWriteOpts & VolumeApiOpts. The 2 lines are merged into one describing the intersection. TemplateBase.build documents tags, which the signature does not take, and does not document config, which it does.
|
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed against TASTE.md (e2b-dev/sdk-harness), focusing on the docstring rules this PR touches: T-69 (docstrings are API: document params, defaults, failure modes), T-70 (JSDoc tags), T-72 (@returns must not restate the method name), plus T-12 (acronym casing in prose) and T-45/T-47 (timeout knobs documented).
3 violations, all in packages/js-sdk/src/volume/index.ts — the two phantom options params are correctly removed, but collapsing them into @param opts <domain> and connection options leaves the actual knobs (uid, gid, mode, force, requestTimeoutMs) undocumented, which is what T-69 asks for.
Not tied to a changed line: in packages/js-sdk/src/template/index.ts the added @param config is accurate and the tags removal is right; @param options Build configuration options there is equally content-free, but build() is private so T-69 doesn't bite. Also worth noting for a follow-up (pre-existing, not this diff): makeDir/writeFile take an inline intersection VolumeWriteOpts & VolumeApiOpts rather than a named exported option type (T-23), which is part of why the opts docstring has nothing to link to.
| * @param path path to the directory to create. | ||
| * @param options directory creation options. | ||
| * @param opts connection options. | ||
| * @param opts directory creation and connection options. |
There was a problem hiding this comment.
T-69 (docstrings are part of the API — every public method documents its parameters, defaults and failure modes) and T-72 (a @returns that restates the method name documents nothing — here it is missing entirely while the method returns VolumeEntryStat).
Dropping the phantom options param is right, but "directory creation and connection options" names no knob the caller can act on: opts is VolumeWriteOpts & VolumeApiOpts, i.e. uid/gid/mode, force (create missing parents), and the connection surface (requestTimeoutMs, headers, proxy, domain).
| * @param opts directory creation and connection options. | |
| * @param opts directory metadata (`uid`, `gid`, `mode`), `force` to also create missing parent directories, and connection options such as `requestTimeoutMs` (default 60_000), `headers`, and `proxy`. | |
| * | |
| * @returns stat of the created directory: its name, path, type, size, and access/modification/creation times |
| * @param data data to write to the file. Data can be a string, `ArrayBuffer`, `Blob`, or `ReadableStream`. Outside the browser, `ReadableStream` data is streamed to the API instead of being buffered in memory. | ||
| * @param options file creation options. | ||
| * @param opts connection options. | ||
| * @param opts file creation and connection options. |
There was a problem hiding this comment.
T-69 — same issue as makeDir: "file creation and connection options" documents nothing. force here means overwrite an existing file (not "create parents" as in makeDir), and this method defaults requestTimeoutMs to FILE_TIMEOUT_MS (1 hour) rather than the usual 60s — exactly the kind of default T-69/T-47 want stated in the docstring, and the kind of same-name/different-meaning flag a docstring is the only place to settle.
| * @param opts file creation and connection options. | |
| * @param opts file metadata (`uid`, `gid`, `mode`), `force` to overwrite an existing file, and connection options such as `requestTimeoutMs` (defaults to 1 hour for file writes), `headers`, and `proxy`. |
| * @param opts connection options. | ||
| * @param opts file creation and connection options. | ||
| * | ||
| * @returns information about the written file |
There was a problem hiding this comment.
T-72 — a @returns that restates the method name documents nothing: "information about the written file" is the value's type spelled in English. Say which fields come back (VolumeEntryStat), the way getInfo is expected to enumerate what it returns.
| * @returns information about the written file | |
| * @returns stat of the written file: its name, path, type, size, and access/modification/creation times |
|
Hey @karpovantonme would you be able to follow Devin's guidance? Otherwise we will have to close the PR as stale. |
2 participants
Volume.makeDirandVolume.writeFileeach documentoptionsandopts, and the signature takes 1 merged parameter,opts?: VolumeWriteOpts & VolumeApiOpts. I merged the 2 doc lines instead of dropping one, since both halves are still true of the single argument.TemplateBase.builddocumentstags, which is not in the signature, and says nothing aboutconfig, which is. I added a line forconfig; drop that hunk if you would rather leave the internal method as it was.Comments only, no behaviour change. Found by a checker I wrote that compares every documented
@paramname against the signature under it. It read 183 of the 185 documented comments in this repository and these 3 were all it had to say.