Skip to content

chore: move initGenesisChunks call into GetGenesisChunks#473

Merged
tzdybal merged 3 commits intoevstack:mainfrom
distractedm1nd:lazy-initGenesisChunks
Jul 28, 2022
Merged

chore: move initGenesisChunks call into GetGenesisChunks#473
tzdybal merged 3 commits intoevstack:mainfrom
distractedm1nd:lazy-initGenesisChunks

Conversation

@distractedm1nd
Copy link
Copy Markdown
Contributor

Resolves #470 (I think?)

Should the error handling from initGenesisChunks be kept inside GetGenesisChunks, or be passed to GenesisChunked ?

@tzdybal
Copy link
Copy Markdown
Contributor

tzdybal commented Jul 22, 2022

Error should be propagated to GenesisChunked.

@adlerjohn adlerjohn added the T:code-hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. label Jul 23, 2022
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 23, 2022

Codecov Report

Merging #473 (9550cf2) into main (5e77f68) will decrease coverage by 0.01%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##             main     #473      +/-   ##
==========================================
- Coverage   55.17%   55.15%   -0.02%     
==========================================
  Files          48       48              
  Lines        8799     8802       +3     
==========================================
  Hits         4855     4855              
- Misses       3191     3193       +2     
- Partials      753      754       +1     
Impacted Files Coverage Δ
rpc/client/client.go 45.94% <25.00%> (-0.67%) ⬇️
node/node.go 65.32% <50.00%> (+1.50%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

Copy link
Copy Markdown
Contributor

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

Can the documentation around the three affected functions be improved here? It's not obvious to be how they differ:

  1. returns chunked version of genesis.
  2. returns given chunk of genesis.

You really wouldn't be able to tell these two descriptions apart were it not for the parameter, but what the functions do is completely different. What are their assumptions around initialization, what are they parameters and return values, etc.

Comment thread node/node.go
return n.genChunks
func (n *Node) GetGenesisChunks() ([]string, error) {
err := n.initGenesisChunks()
return n.genChunks, err
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Stylistic nit, but would prefer explicitly handling the error case here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, @tzdybal prefers the other way - which way should I take?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Or did I misunderstand something about how the error should be handled 😅

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh I meant instead of returning return n.genChunks, err, return return n.genChunks, nil and do an explicit

if err != nil {
    return nil, err
}

before that.

@distractedm1nd distractedm1nd force-pushed the lazy-initGenesisChunks branch from df50bb3 to 9550cf2 Compare July 28, 2022 05:24
@tzdybal tzdybal merged commit 30fa397 into evstack:main Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T:code-hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Call initGenesisChunks in a lazy way

4 participants