Conversation
Codecov Report
@@ Coverage Diff @@
## master #6077 +/- ##
==========================================
+ Coverage 60.76% 60.99% +0.22%
==========================================
Files 275 275
Lines 25345 25345
==========================================
+ Hits 15401 15459 +58
+ Misses 8341 8294 -47
+ Partials 1603 1592 -11
|
alexanderbez
left a comment
There was a problem hiding this comment.
ACK -- curious, why is this breaking?
|
I think because it renames the "behaviour" package. Maybe we should walk all of this back for now; I did not mean to trigger all of this with the linter addition |
|
Ahhh, right! That's pretty unfortunate. Well do we rip the bandaid off now or just never do it? |
|
I believe the behaviour package will most likely be made redundant by the p2p refactor which should define it's own way of handling peers. Also this package is currently only used by the v2 blockchain reactor |
|
If it's only used by the blockchain v2 reactor, maybe it's OK to rip the bandaid off... |
|
It's all internal (or at least should be) to v2 so maybe we could actually make it internal and move the behavior stuff into the v2 reactor. Then at a later point if we decide to integrate other reactors with the peer manager we can slide out the behavior stuff for the new system |
|
Okay. Down the hatch! Let's do it! |
|
I think this merits a changelog entry as well. |
| // Reporter provides an interface for reactors to report the behaviour | ||
| // Reporter provides an interface for reactors to report the behavior | ||
| // of peers synchronously to other components. | ||
| type Reporter interface { |
There was a problem hiding this comment.
These are still publicly exposed interfaces. Should I make them private?
There was a problem hiding this comment.
If they're moved into internal, it won't matter I believe.
There was a problem hiding this comment.
ah right but it isn't in an internal folder. I've just moved it to be a part of the blockchain/v2 package
There was a problem hiding this comment.
It seems we have two options (assuming they're only used within blockchain v2):
- Use an
internalpackage and keep it exported - Make these types private
There was a problem hiding this comment.
Yup. What's your preference?
There was a problem hiding this comment.
I have none, but keep in mind:
Option (1):
- leans towards the approach we're ultimately going to take towards Tendermint 1.0, so it's more "idiomatic"
- the PR diff will be larger, but I guess this isn't a big deal
Option (2):
- probably less work for you
- smaller PR diff
There was a problem hiding this comment.
Let's do Option 1, but as a separate PR from all these spelling changes. I can revert the linter change if it's causing trouble between now and then.
There was a problem hiding this comment.
Cool shall I revert the last couple of commits and we get this in as the lint pleaser PR and then I will open a new one internalizing the behavior package
There was a problem hiding this comment.
Yep. I'll remove the linter in a separate PR as well
There was a problem hiding this comment.
I don't think you will need to
d452ed3 to
5c860ea
Compare
|
So apart from the behavior package the only "code" thing we are changing is renaming the |
|
@cmwaters can you also add a changelog pending entry for the renamed code? |
|
Done it here: #6100 |
Closes: #6030