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

Add JSDocs to everything #56

Merged
merged 6 commits into from Mar 17, 2024
Merged

Add JSDocs to everything #56

merged 6 commits into from Mar 17, 2024

Conversation

aryaemami59
Copy link
Collaborator

This PR:

  • Adds inline documentation to everything inside of the library in the form of JSDocs.

@mmkal
Copy link
Owner

mmkal commented Mar 13, 2024

@aryaemami59 thanks so much, this is great! I am planning on releasing v1 very soon, so this will be great to have.

I've read through these and they look very helpful and well-written to me. I might look to tweak them/see if some of the examples can be auto-generated to make sure they stay up to date, but this is a fantastic starting point.

There's a small conflict in src/index.ts, could you resolve it? After that, anything else needed before marking this as ready?

@aryaemami59
Copy link
Collaborator Author

@mmkal thank you, I will resolve the conflicts, not sure if there was anything else I wanted to add but I'm going to mark at as ready and if I can think of anything else to add, I'll open up a separate PR.

@aryaemami59 aryaemami59 marked this pull request as ready for review March 13, 2024 22:21
@aryaemami59
Copy link
Collaborator Author

@mmkal Please let me know if there is anything you want me to change. I'd be happy to do it.

@mmkal
Copy link
Owner

mmkal commented Mar 15, 2024

@aryaemami59 took a closer look through and some changes I think in a couple of cases these can be trimmed down a little bit. I would prefer to only have jsdoc that adds helpful, non-obvious information, rather than have 100% doc coverage. If it's repeating information that can be gleaned from the type/value itself, it risks drifting from the implementation it mirrors, as well as being noisy in the meantime. So:

  1. Let's remove the @ template T ... tags. In I think every case, it's pretty obvious what the typearg represents
  2. The docs on the symbol type helpers don't add anything IMO. const inverted = ..., const expectSymbol = ... etc. They are a quirky implementation detail for the sake of helpful error messages, there's no use case for dealing with the directly. A comment above the block that defines them along those lines would be useful though

The ones on the methods that are part of the public API are super useful. Thanks again!

@aryaemami59
Copy link
Collaborator Author

@mmkal Sounds good, I will make the requested adjustments. Thanks for the feedback.

@aryaemami59 aryaemami59 marked this pull request as draft March 15, 2024 21:59
@aryaemami59 aryaemami59 marked this pull request as ready for review March 16, 2024 00:10
Copy link
Owner

@mmkal mmkal left a comment

Choose a reason for hiding this comment

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

Looks great 👍 👍 👍

@mmkal mmkal merged commit 3daba59 into mmkal:main Mar 17, 2024
1 check passed
@aryaemami59 aryaemami59 deleted the add-jsdocs branch March 17, 2024 23:09
kodiakhq bot pushed a commit to X-oss-byte/Nextjs that referenced this pull request Apr 1, 2024
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [expect-type](https://togithub.com/mmkal/expect-type) | [`0.18.0` -> `0.19.0`](https://renovatebot.com/diffs/npm/expect-type/0.18.0/0.19.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/expect-type/0.19.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/expect-type/0.19.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/expect-type/0.18.0/0.19.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/expect-type/0.18.0/0.19.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>mmkal/expect-type (expect-type)</summary>

### [`v0.19.0`](https://togithub.com/mmkal/expect-type/releases/tag/0.19.0)

[Compare Source](https://togithub.com/mmkal/expect-type/compare/0.18.0...0.19.0)

#### What's Changed

-   Fix `.omit()` to work similarly to `Omit` by [@&#8203;aryaemami59](https://togithub.com/aryaemami59) in [mmkal/expect-type#54
-   Add JSDocs to everything by [@&#8203;aryaemami59](https://togithub.com/aryaemami59) in [mmkal/expect-type#56
-   Remove `test` import in `README.md` by [@&#8203;aryaemami59](https://togithub.com/aryaemami59) in [mmkal/expect-type#65

**Full Changelog**: mmkal/expect-type@0.18.0...0.19.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/X-oss-byte/Nextjs).
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.

None yet

2 participants