-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename reader to follower #2867
Conversation
d042957
to
bdec07f
Compare
bdec07f
to
736843e
Compare
736843e
to
54472c0
Compare
The changes one the network side looks good to me; I wonder if the consensus report needs to be update as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in the Meet, the alignment still needs to be updated 🙂
Apparently, the current version doesn't contain any references to readers 🙂. I have actually written the section on readers in the last week, it will appear shortly in #2853 (and I'll rename them to followers first). |
5613805
to
8275df6
Compare
|
||
checkIfReaderExists :: CPS.ReaderId -> Model blk | ||
checkIfFollowerExists :: CPS.FollowerId -> Model blk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just sharing an observation. Alignment breaks like this one are the one reason we chose to not align ->
with ::
in the ouroboros-consensus/doc/StyleGuide.md
. (Note that we also chose to keep ::
on the first line for other reasons.)
So, as your judgement call, if you feel like you're fixing up this kind of alignment for almost all of the signatures in some module, feel free to also change all those signatures in such a module to adhere to the StyleGuide.md
. EG in this case (depending how many of the other signatures in this file you're already touching), it'd change to:
checkIfFollowerExists ::
CPS.FollowerId
-> Model blk
-> a
-> Either (ChainDbError blk) a
(I'm suggesting preserving this style despite the StyleGuide.md
because we don't want to inflate the diff too much with signatures that would otherwise be unchanged by this PR. Also, the "existing style" of a module is generally to be preserved, despite the StyleGuide.md
, unless we have good reason.)
Somewhat separately: in the case of a rename, I would be tempted to have separate commits for the rename and for the alignment-fixup. Not crucial, but I think that would make it easier for me personally to review, for example.
0f59872
to
bd5a270
Compare
I see some strays left. Here is an example from each module in which I'm getting hits.
My command was So close! |
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/ChainDB/StateMachine.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/ChainDB/StateMachine.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/ChainDB/StateMachine.hs
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/ChainDB/StateMachine.hs
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/ChainDB/StateMachine.hs
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/ChainDB/StateMachine.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/MiniProtocol/ChainSync/Server.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Storage/ChainDB/Impl.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Storage/ChainDB/Impl/Follower.hs
Outdated
Show resolved
Hide resolved
ouroboros-network/src/Ouroboros/Network/MockChain/ProducerState.hs
Outdated
Show resolved
Hide resolved
Wow! That was a lot of lines! I found a few issues. You were very thorough! Again, I really don't think Thomas and I realized how big this would be. Thanks for persevering. It's also possible that some of my comments were off base -- I wasn't looking at the context in the module, just the diff on GitHub. So feel free to shut me down in some of these comment threads. EG Maybe I applied the Consensus FYI, I can re-review the fix ups on this first thing tomorrow -- ie by 3 or 4pm Central European. |
The last step will be to squash the commits. Like we chatted about, one big commit is fine for this 👍 |
Ha! I know how they got here. After recent patch to memleak some of those variables were introduced and I did not notice that while rebasing. Thanks for the good catch! |
but I also see there were other few that I've missed |
bd5a270
to
de2049d
Compare
@nfrisby I've added few commits to this PR. Please review them. I will squash everything once I get the final 👍 on this. |
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/ChainDB/StateMachine.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/ChainDB/StateMachine.hs
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/ChainDB/StateMachine.hs
Outdated
Show resolved
Hide resolved
6aee120
to
e3872ab
Compare
e3872ab
to
3cefd20
Compare
I rebased onto master (no conflicts, it was automatic) and fixed up a couple more alignment hiccups. Some of those hiccups you and I had fixed on our call yesterday, so I'm guessing we accidentally lost them somehow. We probably jumped to the squash without committing first our staged changes? Anyway, my fixup commit is entirely whitespace. If I show it with the whitespace diff suppressed, it's empty:
I'm going to squash it now and then merge! 🙌 |
Fixes #2833 ouroboros-network has its own set of rules we have to adhere to, thus the differences Question is: in Test/ChainProducerState.hs why not going from r to f (as in other files): 1. the r name was somewhat incorrect in the first place because it was referencing a ReaderState/FollowerState and in other places it was referencing just the Reader/FollowerState 2. the f name was already taken in few places Thus to make things a bit more consistent I've decided to go with /s/r/fs. Not that for the pattern matching on ChainProducerState I've also made a decision on renaming the rs to cfs (because that is a chain follower state) and nrid to cnfid (because that is a chain new follower id)
3cefd20
to
aee2d1f
Compare
bors r+ |
Build succeeded: |
Fixes #2833