Skip to content

docs: add standard library types section with examples, improve grouping#218

Open
chenerdogs-star wants to merge 1 commit into
piotrwitek:masterfrom
chenerdogs-star:master
Open

docs: add standard library types section with examples, improve grouping#218
chenerdogs-star wants to merge 1 commit into
piotrwitek:masterfrom
chenerdogs-star:master

Conversation

@chenerdogs-star

Copy link
Copy Markdown

Improve README documentation of standard library types.

Changes

  • Add new Standard Library Types (built-in) section with Record, Parameters, ConstructorParameters and use-case examples
  • Add missing usage examples to Partial, Readonly, NonNullable, Exclude, Extract, ReturnType, InstanceType
  • Add Optional alias for Partial type
  • Rename Operations on objects section to Operations on object keys for clarity
  • Remove duplicate ReturnType/InstanceType entries from Special operators TOC

Fixes #51

- Add new 'Standard Library Types (built-in)' API section with Record,
  Parameters, ConstructorParameters and use-case examples
- Add usage examples to Partial, Readonly, NonNullable, Exclude, Extract
- Add Optional alias for Partial type
- Rename 'Operations on objects' to 'Operations on object keys'
- Remove duplicate ReturnType/InstanceType entries from Special operators TOC

Fixes piotrwitek#51

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request updates the README.md to include a new 'Standard Library Types' section, adds usage examples for various utility types, and reorganizes the Table of Contents. Feedback highlights several issues including duplicate TOC entries, broken internal links, and an inaccurate section renaming. Additionally, the reviewer pointed out a misleading usage example for the Record type and a potential naming conflict with the Optional alias, while suggesting consistent use of the (built-in) suffix for standard library headers.

Comment thread README.md
Comment on lines +108 to +112
* [`Partial<T>`](#partialt) _(built-in)_
* [`Required<T>`](#requiredt) _(built-in)_
* [`Readonly<T>`](#readonlyt) _(built-in)_
* [`Pick<T, K>`](#pickt-k-built-in) _(built-in)_
* [`Omit<T, K>`](#omitt-k-built-in) _(built-in)_

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The new "Standard Library Types" section introduces several duplicate entries in the Table of Contents. Partial, Required, Readonly, Pick, and Omit are already listed under the "Object operators" section (lines 132-140). To maintain a clean and organized TOC, these should be removed from the "Object operators" section, similar to how ReturnType and InstanceType were removed from "Special operators".

Additionally, some of the new links are broken or inconsistent:

  • #requiredt (line 109) has no corresponding header (the header at line 757 is #requiredt-k).
  • #omitt-k-built-in (line 112) does not match the header at line 566.

Comment thread README.md
[⇧ back to top](#table-of-contents)

## Operations on objects
## Operations on object keys

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Renaming this section to "Operations on object keys" is slightly inaccurate. While some utilities here return a union of keys (like FunctionKeys), many others return transformed object types (like Omit, Pick, Overwrite, and Assign). The previous name "Operations on objects" was more representative of the section's overall content.

Suggested change
## Operations on object keys
## Operations on objects

Comment thread README.md
Comment on lines +560 to +561
// Expect: { name: string; age: number; visible: boolean; }
type Result = Record<'name' | 'age' | 'visible', string | number | boolean>;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The expected type in this comment is inaccurate. Record<K, T> maps every key in K to the type T. In this example, every property (name, age, visible) would have the full union type string | number | boolean. A clearer example for documentation would use a single type for the values to demonstrate the mapping clearly.

Suggested change
// Expect: { name: string; age: number; visible: boolean; }
type Result = Record<'name' | 'age' | 'visible', string | number | boolean>;
// Expect: { name: string; age: string; visible: string; }
type Result = Record<'name' | 'age' | 'visible', string>;

Comment thread README.md

[⇧ back to top](#table-of-contents)

### `Omit<T, K>`

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

To fix the broken TOC link at line 112 and maintain consistency with other types in the "Standard Library Types" section (like Record or Parameters), the (built-in) suffix should be added to this header.

Suggested change
### `Omit<T, K>`
### `Omit<T, K>` _(built-in)_

Comment thread README.md

Make all properties of object type optional

Alias: `Optional<T>`

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Adding Optional<T> as an alias for Partial<T> here is confusing because this library already defines a custom Optional<T, K> utility (documented at line 479). While they behave similarly when the second argument is omitted, Partial<T> is a standard TypeScript built-in, and introducing a conflicting name in the documentation may lead to ambiguity for users.

Comment thread README.md

[⇧ back to top](#table-of-contents)

### `ReturnType<T>`

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

For consistency with other standard library types documented in this PR, this header should include the (built-in) suffix.

Suggested change
### `ReturnType<T>`
### `ReturnType<T>` _(built-in)_

Comment thread README.md

[⇧ back to top](#table-of-contents)

### `InstanceType<T>`

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

For consistency with other standard library types documented in this PR, this header should include the (built-in) suffix.

Suggested change
### `InstanceType<T>`
### `InstanceType<T>` _(built-in)_

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve organization and add missing examples in API documentation

1 participant