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.

Sign up for GitHub

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

Jump to bottom

docs(adr): Public API #506

Merged
merged 25 commits into from
Nov 12, 2022
Merged

docs(adr): Public API #506

merged 25 commits into from
Nov 12, 2022

Conversation

Wondertan
Copy link
Member

@Wondertan Wondertan commented Mar 8, 2022 •
edited
Loading

image

TODO

  • Include API and Node construction
  • Document each method
  • Add method params
    - [ ] Consquances

@Wondertan Wondertan added the docs:adr ADR label Mar 8, 2022
@Wondertan Wondertan requested a review from liamsi as a code owner March 8, 2022 12:15
@Wondertan Wondertan self-assigned this Mar 8, 2022
@Wondertan Wondertan requested a review from renaynay as a code owner March 8, 2022 12:15
renaynay
renaynay reviewed Mar 8, 2022
View reviewed changes
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

I like the idea of modules vs services and everything LGTM at first glance. Made some comments.

docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
liamsi
liamsi reviewed Mar 23, 2022
View reviewed changes
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
@renaynay renaynay mentioned this pull request Mar 29, 2022
Closed
@Wondertan Wondertan mentioned this pull request Apr 15, 2022
Closed
2 tasks
renaynay
renaynay reviewed Apr 19, 2022
View reviewed changes
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

another round of comments.

docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
docs/adr/adr-007-public-api.md Outdated Show resolved Hide resolved
@Wondertan
Copy link
Member Author

SharesModule is going to be renamed to DataModule as per discussion with @renaynay

Wondertan and others added 2 commits April 20, 2022 14:54
@Wondertan @renaynay
Update docs/adr/adr-007-public-api.md
71dc7d2 
Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>
@Wondertan @renaynay
Update docs/adr/adr-007-public-api.md
0431aaa 
Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>
@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2022 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (main@6592f3b). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #506   +/-   ##
=======================================
  Coverage        ?   54.88%           
=======================================
  Files           ?      178           
  Lines           ?    10689           
  Branches        ?        0           
=======================================
  Hits            ?     5867           
  Misses          ?     4246           
  Partials        ?      576

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

This was referenced Apr 26, 2022
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@Wondertan
Copy link
Member Author

Converting to Draft as WIP

@Wondertan Wondertan marked this pull request as draft April 26, 2022 15:34
This was referenced Jul 25, 2022
Closed
Closed
Closed
@renaynay renaynay added the kind:misc Attached to miscellaneous PRs label Aug 3, 2022
renaynay
renaynay requested changes Oct 26, 2022
View reviewed changes
docs/adr/adr-009-public-api.md Outdated Show resolved Hide resolved
distractedm1nd
distractedm1nd previously approved these changes Nov 10, 2022
View reviewed changes
Copy link
Collaborator

@distractedm1nd distractedm1nd left a comment

Choose a reason for hiding this comment

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

merge please

@Wondertan Wondertan enabled auto-merge (squash) November 10, 2022 17:03
distractedm1nd
distractedm1nd previously approved these changes Nov 11, 2022
View reviewed changes
MSevey
MSevey reviewed Nov 11, 2022
View reviewed changes
Copy link
Member

@MSevey MSevey left a comment

Choose a reason for hiding this comment

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

LGTM, not sure I have sufficient historical context to fully approve, will leave that to others.

renaynay
renaynay previously approved these changes Nov 12, 2022
View reviewed changes
@renaynay renaynay mentioned this pull request Nov 12, 2022
Closed
@renaynay renaynay dismissed stale reviews from distractedm1nd and themself via 6b12b08 November 12, 2022 08:05
liamsi
liamsi approved these changes Nov 12, 2022
View reviewed changes
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Left some minor comments. My understanding is that this will be merged and any outstanding comments will be addressed in a followup PR.

docs/adr/adr-009-public-api.md Show resolved Hide resolved
docs/adr/adr-009-public-api.md Show resolved Hide resolved
docs/adr/adr-009-public-api.md Show resolved Hide resolved
docs/adr/adr-009-public-api.md Show resolved Hide resolved
docs/adr/adr-009-public-api.md Show resolved Hide resolved
docs/adr/adr-009-public-api.md Show resolved Hide resolved
docs/adr/adr-009-public-api.md Show resolved Hide resolved
@Wondertan Wondertan merged commit bd13b76 into main Nov 12, 2022
@Wondertan Wondertan deleted the hlib/adr-007 branch November 12, 2022 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MSevey MSevey MSevey left review comments

@rootulp rootulp rootulp left review comments

@Bidon15 Bidon15 Bidon15 left review comments

@tzdybal tzdybal tzdybal left review comments

@renaynay renaynay renaynay approved these changes

@distractedm1nd distractedm1nd distractedm1nd approved these changes

@liamsi liamsi liamsi approved these changes

@vgonkivs vgonkivs Awaiting requested review from vgonkivs

@adlerjohn adlerjohn Awaiting requested review from adlerjohn adlerjohn is a code owner

Assignees

@Wondertan Wondertan

@renaynay renaynay

Labels
docs:adr ADR kind:misc Attached to miscellaneous PRs
Projects
No open projects
Celestia Node
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@Wondertan @codecov-commenter @renaynay @tzdybal @Bidon15 @rootulp @liamsi @MSevey @distractedm1nd