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

Improving doc for Record.ts #1574

Merged
merged 10 commits into from Oct 27, 2021
Merged

Improving doc for Record.ts #1574

merged 10 commits into from Oct 27, 2021

Conversation

fmpanelli
Copy link
Contributor

Added documentation comments for most members of Record.ts. No changes to the code.

Added doc comments for most members
of Record.ts. No changes to the code.
@fmpanelli fmpanelli marked this pull request as ready for review August 31, 2021 20:08
docs/modules/Record.ts.md Outdated Show resolved Hide resolved
docs/modules/Record.ts.md Show resolved Hide resolved
fmpanelli and others added 2 commits September 1, 2021 05:48
Co-authored-by: Eric Crosson <EricCrosson@users.noreply.github.com>
Applying suggestions from @EricCrosson for:

- the overview comment comment
- consistent spelling for '2' vs 'two'
Copy link
Contributor

@samhh samhh left a comment

Choose a reason for hiding this comment

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

I got tired so am ending my review here, a testament to the amount of work you've put into this. Documentation like this is extremely valuable to this ecosystem, thank you so much for contributing it.

docs/modules/Record.ts.md Outdated Show resolved Hide resolved
docs/modules/Record.ts.md Outdated Show resolved Hide resolved
docs/modules/Record.ts.md Outdated Show resolved Hide resolved
docs/modules/Record.ts.md Outdated Show resolved Hide resolved
docs/modules/Record.ts.md Outdated Show resolved Hide resolved
docs/modules/Record.ts.md Outdated Show resolved Hide resolved
@fmpanelli
Copy link
Contributor Author

I got tired so am ending my review here, a testament to the amount of work you've put into this. Documentation like this is extremely valuable to this ecosystem, thank you so much for contributing it.

Thank you so much for every feedback. I'll make one commit trying to address all the points you raised.

@raveclassic
Copy link
Collaborator

This is awesome! Thank you for your hard work on this, @fmpanelli!

@raveclassic
Copy link
Collaborator

@fmpanelli @samhh Is there anything else you guys would like to add here?

Copy link
Contributor

@alex-ketch alex-ketch left a comment

Choose a reason for hiding this comment

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

Thanks so much for adding this @fmpanelli, it's tremendously helpful 💖
I've left a few suggestions based on a quick read through for any typos.

docs/modules/Record.ts.md Outdated Show resolved Hide resolved
docs/modules/Record.ts.md Show resolved Hide resolved
docs/modules/Record.ts.md Show resolved Hide resolved
src/Record.ts Outdated Show resolved Hide resolved
fmpanelli and others added 2 commits September 3, 2021 17:49
Co-authored-by: Alex Ketch <alex-ketch@users.noreply.github.com>
Co-authored-by: Alex Ketch <alex-ketch@users.noreply.github.com>
@fmpanelli
Copy link
Contributor Author

I did not expect so much attention and so much feedback ❤️
Thanks, everybody.
I will work on this next week, trying to fix the tslint issues and to incorporate all feedback.

@fmpanelli
Copy link
Contributor Author

I fixed the lint problems (it was enough to remove a couple of trailing spaces).

I improved the documentation further.
I tried to implement all the suggestions. You are welcome to add new ones 😃

@fmpanelli
Copy link
Contributor Author

I just aligned the documentation for ReadonlyRecord.ts, basically copying and adjusting what I had done for Record.ts.

@fmpanelli
Copy link
Contributor Author

Hi, I haven't seen any further comment.
I have no more contribution at the moment.
Shall I do something more? How may we proceed?
Thanks!

@gcanti gcanti merged commit cb9821d into gcanti:master Oct 27, 2021
@gcanti
Copy link
Owner

gcanti commented Oct 27, 2021

Thank you @fmpanelli

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

6 participants