Skip to content

Conversation

@johningve
Copy link
Member

@johningve johningve commented Aug 12, 2022

This PR changes how the module system works. Modules can no longer directly access a module through the "Core" object in this manner: mods.Eventloop(). Instead, modules are expected to declare a local variable that should hold the module, and then call one of the new Get methods to retrieve the module. For example:

var eventLoop *eventloop.Eventloop
mods.Get(&eventLoop)

Additionally, this PR adds a document that describes the ideas behind the module system and why it is necessary, and fixes a couple of mistakes.

@johningve johningve marked this pull request as ready for review August 19, 2022 11:28
@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2022

Codecov Report

Merging #73 (042e6b4) into master (6152764) will increase coverage by 4.88%.
The diff coverage is 78.81%.

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
+ Coverage   57.81%   62.70%   +4.88%     
==========================================
  Files          70       73       +3     
  Lines        6427     7197     +770     
==========================================
+ Hits         3716     4513     +797     
+ Misses       2424     2384      -40     
- Partials      287      300      +13     
Flag Coverage Δ
unittests 62.70% <78.81%> (+4.88%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
consensus/byzantine/byzantine.go 1.92% <0.00%> (-0.21%) ⬇️
crypto/bitfield.go 96.61% <0.00%> (-3.39%) ⬇️
genesis.go 100.00% <ø> (ø)
internal/cli/run.go 16.12% <0.00%> (ø)
leaderrotation/carousel.go 3.77% <0.00%> (-0.49%) ⬇️
leaderrotation/reputation.go 3.07% <0.00%> (-0.26%) ⬇️
metrics/clientlatency.go 2.63% <0.00%> (-0.94%) ⬇️
metrics/plotting/clientlatency.go 0.00% <0.00%> (ø)
metrics/plotting/reader.go 0.00% <ø> (ø)
metrics/plotting/starttimes.go 0.00% <0.00%> (ø)
... and 72 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@johningve
Copy link
Member Author

@meling I decided to drop the explicit modules.Implements[T] declaration for now. The rule now is that mods.Get will give the last compatible value that was added to the modules builder.

@johningve johningve force-pushed the module-refactor branch 2 times, most recently from 33da9e6 to 65ace61 Compare August 22, 2022 08:34
@johningve
Copy link
Member Author

@meling I have myself reviewed all the changes and am happy with what is here. If you think that the changes to the module system look okay, then I think that we can merge it

This removes the need to declare what "interface" or type a module
provides to the module system, thus being more in line with the "Go way"
of implicit interfaces. The module system looks for a module with a
compatible type. If multiple modules implement the same type, the module
system chooses the one that was added to the builder last.
@johningve johningve force-pushed the module-refactor branch 2 times, most recently from e622d02 to 040b049 Compare August 22, 2022 11:57
@johningve johningve merged commit 5196107 into master Aug 23, 2022
@johningve johningve deleted the module-refactor branch August 23, 2022 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants