celestia-app icon indicating copy to clipboard operation
celestia-app copied to clipboard

specs: identify inconsistencies in implementation

Open rootulp opened this issue 3 years ago • 9 comments

Discussed offline with @evan-forbes and we need to do a pass of existing specs, identify inconsistencies, fix them, and then migrate specs to individual repos.

Inconsistencies

  • https://github.com/celestiaorg/celestia-specs/issues/211
  • https://github.com/celestiaorg/celestia-app/issues/728
  • https://github.com/celestiaorg/celestia-app/issues/883

rootulp avatar Sep 14 '22 19:09 rootulp

I'm also ok with doing this as we move chunks into each repo. We can then examine and edit the specs in those PRs, which might even add more context and links to code.

evan-forbes avatar Sep 15 '22 10:09 evan-forbes

Option A:

  1. Identify inconsistencies
  2. Fix them in this repo
  3. Move the updated specs to individual repos

Option B:

  1. Move the specs to individual repos
  2. Identify and fix inconsistencies in the PRs that move specs

I'm leaning towards Option A because I think that will lead to smaller, atomic PRs.

rootulp avatar Sep 15 '22 15:09 rootulp

I'm leaning towards option B since it's a forcing function. In the current repo, there aren't enough eyes paid to the specs since it's one step removed.

adlerjohn avatar Sep 18 '22 13:09 adlerjohn

I'm also more in favour of option B. Additionally to what John said, I believe that if the specs are closer to the actual code that, they would rather be maintained as well.

liamsi avatar Sep 18 '22 13:09 liamsi

Since we're going with Option B we no longer need this issue to identify inconsistencies.

rootulp avatar Sep 21 '22 17:09 rootulp

Since we're going with Option B we no longer need this issue to identify inconsistencies.

? I think we might afaiu. I believe @adlerjohn will copy over the portions of the spec that are relevant to the app sometime soon, and then we can identify and fix inconsistencies.

evan-forbes avatar Sep 21 '22 17:09 evan-forbes

This was a miscommunication on my part, but there was also option C, which was only discussed on slack

  • copy over the relevant specs
  • identify and fix portions of the spec as we find them

we still get atomic PRs

evan-forbes avatar Sep 21 '22 17:09 evan-forbes

I should've included Option C above but yes I'm on-board because then we still get atomic PRs in individual repos to fix specs. Given Option C, we'll likely want a unique "Identify inconsistencies between specs and implementation" issue per repo where specs are migrated to.

rootulp avatar Sep 21 '22 18:09 rootulp

I'm leaning towards option B since it's a forcing function. In the current repo, there aren't enough eyes paid to the specs since it's one step removed.

I'll still obvi defer to the author on this. Regardless of what we pick, we'll likely be doing C forever anywho

evan-forbes avatar Sep 21 '22 18:09 evan-forbes

Subsumed by #650

adlerjohn avatar Nov 01 '22 15:11 adlerjohn