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

typings/docs: All typings and documentation fixes #42

Merged
merged 201 commits into from
Feb 19, 2020
Merged

Conversation

bsian03
Copy link
Contributor

@bsian03 bsian03 commented Jan 16, 2020

Adds proper JSDoc/TypeScript typings up to date as of PR

index.d.ts Outdated Show resolved Hide resolved
@Khaaz
Copy link
Owner

Khaaz commented Jan 17, 2020

Thanks a lot for the PR, I also appreciate the various jsdoc changes, typos and improvement.
For all typings, i'll let you discuss with Null or anyone, and i'll go with it. I really appreciate the huge effort.
You can mark this as draft PR while it's a WIP if you want.

@Khaaz
Copy link
Owner

Khaaz commented Jan 17, 2020

Just wondering why it doesn't run CI on the PR though

@bsian03 bsian03 changed the title Mainly TS typings WIP: Mainly TS typings Jan 17, 2020
@bsian03
Copy link
Contributor Author

bsian03 commented Jan 17, 2020

You know I completely forgot this wasn't Gitlab, I don't think there's another way of marking WIP/draft without closing and reopening

@Khaaz
Copy link
Owner

Khaaz commented Jan 17, 2020

yeah i actually thought you could change it to draft after it was already created but guess it's not possible :')

Copy link
Owner

@Khaaz Khaaz left a comment

Choose a reason for hiding this comment

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

Few things really especially considering the PR size.
Some of my comments needs to be taken care of with particular attention though.

I mostly noticed some inconsistency in Library, and I didn't comment everything there.
Lot of missing @memberof statement, sometimes there are return type, sometimes there are not?

Last thing, regenerate a lockfile if you want, but we need the lockfile to be commited

src/Database/InMemoryProvider.js Outdated Show resolved Hide resolved
src/Database/JSON/JsonManager.js Show resolved Hide resolved
src/Database/JSON/JsonManager.js Show resolved Hide resolved
src/Database/JSON/JsonManager.js Show resolved Hide resolved
src/Database/JsonProvider.js Outdated Show resolved Hide resolved
src/Structures/Stores/ARegistry.js Show resolved Hide resolved
src/Structures/Stores/ModuleRegistry.js Outdated Show resolved Hide resolved
src/Utility/Collection.js Outdated Show resolved Hide resolved
src/Utility/Discord/Prompt.js Show resolved Hide resolved
src/Utility/Discord/MessageCollector.js Show resolved Hide resolved
@Khaaz
Copy link
Owner

Khaaz commented Feb 19, 2020

Thanks a lot for this PR.
This is a HUGE work: 201 commits, 10k insertion, 3k removal

I already said that but I am really really thankful for your work and the time you invested in this.

We will continue the work of this PR in a second PR to split index.d.ts in separate files as we discussed.
🙏 🙏 Thanks.

@Khaaz Khaaz merged commit 6c38e4d into Khaaz:dev Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants