go-header icon indicating copy to clipboard operation
go-header copied to clipboard

doc: specs

Open gupadhyaya opened this issue 2 years ago • 14 comments

Overview

Rendered

Checklist

  • [ ] New and updated code has appropriate documentation
  • [ ] New and updated code has new and/or updated testing
  • [ ] Required CI checks are passing
  • [ ] Visual proof for any user facing features like CLI or documentation updates
  • [ ] Linked issues closed with keywords

gupadhyaya avatar Oct 06 '23 15:10 gupadhyaya

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
@@            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.

codecov-commenter avatar Oct 11 '23 14:10 codecov-commenter

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 avatar Oct 24 '23 12:10 gupadhyaya

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

gupadhyaya avatar Oct 24 '23 12:10 gupadhyaya

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

gupadhyaya avatar Oct 27 '23 02:10 gupadhyaya

@gupadhyaya, getting back to this finally

Wondertan avatar Nov 22 '23 15:11 Wondertan

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 avatar Dec 01 '23 10:12 Wondertan

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.

MSevey avatar Dec 01 '23 16:12 MSevey

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

Wondertan avatar Dec 01 '23 16:12 Wondertan

@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.

MSevey avatar Dec 01 '23 18:12 MSevey

@MSevey, which rendered website?

Wondertan avatar Dec 01 '23 18:12 Wondertan

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

Wondertan avatar Dec 01 '23 18:12 Wondertan

@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.

MSevey avatar Dec 01 '23 19:12 MSevey

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?

Wondertan avatar Dec 08 '23 14:12 Wondertan

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.

MSevey avatar Dec 08 '23 16:12 MSevey