chore(consensus): remove irrelevant TODO items #2671
Merged
+0
−6
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
This was referenced Dec 15, 2024
This was referenced Dec 15, 2024
guy-starkware
force-pushed
the
guyn/streams/remove_consensus_message_proposal_from_proto
branch
from
e9559ee to
b67d1bb
Compare
guy-starkware
force-pushed
the
guyn/streams/remove_todos
branch
from
dbb4e5c to
aab774a
Compare
guy-starkware
force-pushed
the
guyn/streams/remove_consensus_message_proposal_from_proto
branch
from
b67d1bb to
6d97140
Compare
guy-starkware
force-pushed
the
guyn/streams/remove_todos
branch
from
aab774a to
cf2c423
Compare
guy-starkware
force-pushed
the
guyn/streams/remove_consensus_message_proposal_from_proto
branch
from
6d97140 to
5e0431f
Compare
guy-starkware
force-pushed
the
guyn/streams/remove_todos
branch
from
cf2c423 to
78bb277
Compare
guy-starkware
force-pushed
the
guyn/streams/remove_consensus_message_proposal_from_proto
branch
from
5e0431f to
1a9ab44
Compare
guy-starkware
force-pushed
the
guyn/streams/remove_todos
branch
from
78bb277 to
61c5340
Compare
guy-starkware
force-pushed
the
guyn/streams/remove_consensus_message_proposal_from_proto
branch
from
1a9ab44 to
4c2a457
Compare
guy-starkware
force-pushed
the
guyn/streams/remove_todos
branch
from
61c5340 to
37db3d4
Compare
guy-starkware
force-pushed
the
guyn/streams/remove_consensus_message_proposal_from_proto
branch
from
4c2a457 to
b21f051
Compare
guy-starkware
force-pushed
the
guyn/streams/remove_todos
branch
from
37db3d4 to
81e97a4
Compare
guy-starkware
force-pushed
the
guyn/streams/remove_consensus_message_proposal_from_proto
branch
from
b21f051 to
006c08f
Compare
guy-starkware
force-pushed
the
guyn/streams/remove_todos
branch
from
81e97a4 to
2c0d923
Compare
asmaastarkware
approved these changes
Dec 22, 2024
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @guy-starkware)
guy-starkware
force-pushed
the
guyn/streams/remove_consensus_message_proposal_from_proto
branch
from
006c08f to
0977142
Compare
guy-starkware
force-pushed
the
guyn/streams/remove_todos
branch
from
2c0d923 to
f936f3e
Compare
guy-starkware
force-pushed
the
guyn/streams/remove_consensus_message_proposal_from_proto
branch
from
0977142 to
ddd8259
Compare
guy-starkware
force-pushed
the
guyn/streams/remove_todos
branch
from
f936f3e to
3eeb458
Compare
matan-starkware
requested changes
Dec 24, 2024
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @guy-starkware)
crates/papyrus_protobuf/Cargo.toml line 12 at r1 (raw file):
[dependencies] # TODO(Guy): Remove after implementing broadcast streams.
what were you supposed to remove?
Code quote:
# TODO(Guy): Remove after implementing broadcast streams.crates/papyrus_protobuf/src/consensus.rs line 116 at r1 (raw file):
{ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { // TODO(guyn): add option to display when message is Fin and doesn't have content (PR #1048)
?
Code quote:
// TODO(guyn): add option to display when message is Fin and doesn't have content (PR #1048)crates/sequencing/papyrus_consensus/src/manager_test.rs line 86 at r1 (raw file):
let mut sender = mock_network.broadcasted_messages_sender; // TODO(guyn): refactor this test to pass proposals through the correct channels.
Already done but you didn't delete the TODO?
Code quote:
// TODO(guyn): refactor this test to pass proposals through the correct channels.
matan-starkware
approved these changes
Dec 24, 2024
There was a problem hiding this comment.
Reviewable status:
guy-starkware
force-pushed
the
guyn/streams/remove_consensus_message_proposal_from_proto
branch
from
ddd8259 to
bd1211a
Compare
guy-starkware
force-pushed
the
guyn/streams/remove_todos
branch
from
3eeb458 to
c9f99a8
Compare
guy-starkware
changed the base branch from
guyn/streams/remove_consensus_message_proposal_from_proto
to
main
|
Artifacts upload workflows: |
guy-starkware
commented
Dec 25, 2024
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A few TODOs that I found that are no longer relevant and are removed.