celestiaorg / go-header

Go library with all the services needed to request, sync and store blockchain headers.
Apache License 2.0
17 stars 16 forks source link

doc: specs #116

Open gupadhyaya opened 8 months ago

gupadhyaya commented 8 months ago

Overview

Rendered

Checklist

codecov-commenter commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (09c272d) 67.81% compared to head (9bd3962) 63.66%. Report is 3 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #116 +/- ## ========================================== - Coverage 67.81% 63.66% -4.15% ========================================== Files 38 39 +1 Lines 3079 3366 +287 ========================================== + Hits 2088 2143 +55 - Misses 843 1058 +215 - Partials 148 165 +17 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

gupadhyaya commented 8 months ago

High-Level Q: What kind of spec is this and who is it targeted for?

It looks pretty Golang implementation-specific, and AFAIK, specs are meta descriptions of protocols and behaviors decoupled from implementation details. Usually, their target is a client implementer who needs to understand how things work. From my understanding, this is excellent code documentation, but not the spec. I wonder if we should aim to extract all the protocol information into an independent doc separating docs from specs or at least make the protocol description part of the documentation.

Anyway, this is awesome, and I am very grateful for this ❤️

I will review this in batches, and the first one is P2P doc review

thanks for your review @Wondertan i am writing this specs from the perspective of the consumer (e.g. rollkit, in future other frameworks). i agree that it includes interface definitions in a specific language (golang), however i don't think it will prevent a non-golang adopter from understanding and consuming it. the whole idea here is to clearly specify the interfaces and implicit/explicit assumptions made. we have followed similar strategy in specifying rollkit.

i agree that we can add a protocol section which tries to summarize the components without getting into the details. but i don't think we should try to make two separate documents (specs and impl details) imho.

if we want, we can convert the golang interface definitions to language independent. let me know.

gupadhyaya commented 8 months ago

@Wondertan let me revise this based on your comments and try to make certain parts golang independent. Will request review soon.

gupadhyaya commented 8 months ago

@Wondertan tried to generalize as much as i can. as discussed, we can revisit regularly and keep improving. wdyt?

Wondertan commented 7 months ago

@gupadhyaya, getting back to this finally

Wondertan commented 7 months ago

So, I added a rendered link to the PR description with rendered specs readme and realized that linking between symlinks defined in the specs folder and the actual specs markdown does not work. Do we really need this symlink redirection? I think we can directly point root specs README to the relevant md files.

cc @MSevey (as I see you set it up this way)

MSevey commented 7 months ago

So, I added a rendered link to the PR description with rendered specs readme and realized that linking between symlinks defined in the specs folder and the actual specs markdown does not work. Do we really need this symlink redirection? I think we can directly point root specs README to the relevant md files.

cc @MSevey (as I see you set it up this way)

@Wondertan what is the actual issue? The rendered readme works fine for me, plus you can see the rendered version in the diff.

The symlinks were a way to keep the specs close to the code to encourage keeping them up to date.

Wondertan commented 7 months ago

@MSevey, try clicking links to Components on the rendered page and you will see

MSevey commented 7 months ago

@MSevey, try clicking links to Components on the rendered page and you will see

oh right, yea we've talked about this internally. The Github UI doesn't support symlinks, but that is fine since we are optimizing for the rendered website and keeping the specs close to the code.

Wondertan commented 7 months ago

@MSevey, which rendered website?

Wondertan commented 7 months ago

I think we should make it work for both GH and the rendered website(or whatever it is) by removing symlinks and linking directly.

MSevey commented 7 months ago

@Wondertan the specs site https://celestiaorg.github.io/go-header/

to make it work with both I would just not link to the specs folder. the specs folder is essentially a wrapper to enable a deployed specs website.

The original discussion the team had came down to whether or not we want the spec md files in the specs folder or close to the code. Since there was a preference for keeping the specs md files close to the code, we set up the specs folder with symlinks to make the website work.

So to make it work on the GH UI we just shouldn't reference the specs folder outside the specs folder.

Wondertan commented 6 months ago

So to make it work on the GH UI we just shouldn't reference the specs folder outside the specs folder.

@MSevey, Do you mean pointing to the code directly like in here?

MSevey commented 6 months ago

So to make it work on the GH UI we just shouldn't reference the specs folder outside the specs folder.

@MSevey, Do you mean pointing to the code directly like in here?

Yea, if a the spec in pkgA wants to reference the spec in pkgB, it should link directly to pkgB not the spec/src/spec/pkgB symlink location.