The Wayback Machine - https://web.archive.org/web/20201122151147/https://github.com/sindresorhus/got/pull/1278
Skip to content

/ got

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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add documentation comments to exported TypeScript types #1278

Merged
merged 17 commits into from Aug 16, 2020
Merged

Add documentation comments to exported TypeScript types #1278

merged 17 commits into from Aug 16, 2020

Conversation

@pmmmwh
Copy link
Contributor

@pmmmwh pmmmwh commented May 21, 2020 •
edited by sindresorhus

This PR address the last missing bit of #758 by adding doc comments for all public exported types.

Most of the wordings are borrowed from the README.

Questions

  1. Is there a way to config VS Code/whatever to conform to sindresorhus/typescript-definition-style-guide#documentation? It is a bit painful (and difficult for me to check consistency) when I constantly have to tab/indent comment blocks by hand 馃う
  2. How should I document merged types like isStream, resolveBodyOnly, responseType, NormalizedOptions, etc.?

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.
  • I have included some tests.
  • If it's a new feature, I have included documentation updates.

Fixes #758
@pmmmwh pmmmwh changed the title Typescript docs Add doc comments to Typescript types May 21, 2020
@pmmmwh pmmmwh changed the title Add doc comments to Typescript types Add doc comments to exported Typescript types May 21, 2020
@pmmmwh pmmmwh mentioned this pull request May 21, 2020
Closed
7 of 9 tasks complete
@pmmmwh
Copy link
Contributor Author

@pmmmwh pmmmwh commented May 21, 2020

Hmm ... I'm not sure why tests are failing. All code added in this PR are within comment blocks.
@sindresorhus
sindresorhus reviewed May 21, 2020
View changes
source/as-promise/types.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jun 6, 2020

Hmm ... I'm not sure why tests are failing.

Just ignore it.
@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jun 6, 2020

Let me know when this is done and ready for review.
@pmmmwh
Copy link
Contributor Author

@pmmmwh pmmmwh commented Jun 10, 2020

Let me know when this is done and ready for review.

I think it is now mostly ready. There are still some parts missing, but they are either:

  • Undocumented in the README,
  • Or I don't really have an idea of whether they need to be documented, or how they should be documented.

A specific case I have some doubts is for function overloads, like GotPaginate or even Got/GotStream. I'm not sure if TS can correctly pick up the documentation for both overloads without duplicating them ...
@pmmmwh pmmmwh marked this pull request as ready for review Jun 10, 2020
pmmmwh added 9 commits May 21, 2020
@pmmmwh
Add typescript docs for core options
2824ee6
@pmmmwh
Add typescript docs for core response
47cb3d1
@pmmmwh
Add missing default for beforeError hook
583242f
@pmmmwh
Add documentation for pagination
f24aca7
@pmmmwh
Better indent doc comments
6be3e3b
@pmmmwh
Add documentation for got.stream APIs
20cef68
@pmmmwh
Indent all doc comments
8b887a9
@pmmmwh
Add documents for core got export
694fd4a
@pmmmwh
Add documents for remaining parts that are available in README
Loading status checks…
02b5533
@pmmmwh pmmmwh force-pushed the pmmmwh:typescript-docs branch from d91796a to 02b5533 Jun 10, 2020
@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jul 3, 2020

I'm not sure if TS can correctly pick up the documentation for both overloads without duplicating them ...

I'm not sure either, but you can test by requiring Got and opening it up in VSCode to see.
@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jul 3, 2020

Can you also update the contribution guidelines to mention that doc changes should also be applied to the doc comments, not just readme.
szmarczak added 4 commits Jul 28, 2020
@szmarczak
Merge branch 'master' into typescript-docs
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub鈥檚 key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
601b26e
@szmarczak
GitHub should leave this...
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub鈥檚 key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
1489353
@szmarczak
Fix linting
Loading status checks…
91ae067
@szmarczak
add missing tabs
Loading status checks…
7cb2612
@szmarczak
Copy link
Collaborator

@szmarczak szmarczak commented Jul 28, 2020

I'm not sure either, but you can test by requiring Got and opening it up in VSCode to see.

I just checked. TS doesn't assume it's the same function with a different name.
@pmmmwh
Copy link
Contributor Author

@pmmmwh pmmmwh commented Jul 28, 2020

I'm not sure either, but you can test by requiring Got and opening it up in VSCode to see.

I just checked. TS doesn't assume it's the same function with a different name.

Should I duplicate the docs or leave it alone? I think this issue also exists for re-exports/types that are used in interfaces as values.

(Sorry I haven't been able to dug into this ...)
@szmarczak
Copy link
Collaborator

@szmarczak szmarczak commented Aug 1, 2020

Should I duplicate the docs or leave it alone?

@sindresorhus Maybe let's duplicate them?
@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Aug 2, 2020

Yeah, I guess we have to duplicate them.
@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Aug 2, 2020

Relevant microsoft/TypeScript#407
pmmmwh added 2 commits Aug 13, 2020
@pmmmwh
Merge remote-tracking branch 'upstream/master' into typescript-docs
d5dd6ea 
# Conflicts:
#	source/as-promise/core.ts
#	source/core/index.ts
@pmmmwh
Document all types (with duplication as needed) according to comments
Loading status checks…
48ee1d1
@pmmmwh
Copy link
Contributor Author

@pmmmwh pmmmwh commented Aug 13, 2020

I finally got time to "finish" this 馃う

I've duplicated all the documentation as necessary, and have checked that most types should be documented.

I've left out some types that are quite sparse/would rarely be used:

  • OptionsOfTextResponseBody and its counterparts plus GotRequestFunction
  • Hook function types (could copy them from the Hooks interface) if needed
  • Cookie Jars (no applicable documentation as the existing ones only describes the signatures)
  • Parts of the retry.calculateDelay API (no applicable documentation)
  • NormalizedOptions and Defaults

Please tell me if you want me to fill in any gaps listed above or need to change anything!
pmmmwh added 2 commits Aug 13, 2020
@pmmmwh
Fix linting
Loading status checks…
961b962
@pmmmwh
Add note in PR template about JSDoc
Loading status checks…
acadb0f
@szmarczak
szmarczak approved these changes Aug 14, 2020
View changes
@szmarczak szmarczak requested a review from sindresorhus Aug 14, 2020
@sindresorhus sindresorhus changed the title Add doc comments to exported Typescript types Add documentation comments to exported TypeScript types Aug 16, 2020
@sindresorhus sindresorhus merged commit eaf1e02 into sindresorhus:master Aug 16, 2020
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Aug 16, 2020

Thank you for finishing this :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants
@pmmmwh @sindresorhus @szmarczak
You can鈥檛 perform that action at this time.