Skip to content
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

comment appned entries #915

Merged
merged 2 commits into from
May 24, 2024
Merged

comment appned entries #915

merged 2 commits into from
May 24, 2024

Conversation

renancloudwalk
Copy link
Contributor

No description provided.

@renancloudwalk renancloudwalk requested a review from a team as a code owner May 24, 2024 00:26
@renancloudwalk renancloudwalk enabled auto-merge (squash) May 24, 2024 00:26
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

2, because the PR involves commenting out a block of code which is straightforward, but it's important to understand the implications of disabling this functionality and ensuring that it does not affect other parts of the system.

🧪 Relevant tests

No

⚡ Possible issues

Possible Bug: The commenting out of the append_entries_to_followers function call may lead to unintended behavior in the consensus mechanism, as this function seems critical for data propagation to followers.

🔒 Security concerns

No

Code feedback:
relevant filesrc/eth/consensus.rs
suggestion      

Consider implementing a feature toggle instead of commenting out code. This allows for easier reactivation of the code and better control over its execution without altering the codebase significantly. [important]

relevant line//XXX match Self::append_entries_to_followers(vec![Entry { index: 0, data: data.clone() }], followers.clone()).await {

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Maintainability
Remove unnecessary 'XXX' comments to enhance code readability

Remove the 'XXX' comments from the code as they are typically used to mark areas that need
work or review. This will make the code cleaner and more professional.

src/eth/consensus.rs [69-77]

-//XXX match Self::append_entries_to_followers(vec![Entry { index: 0, data: data.clone() }], followers.clone()).await {
-//XXX     Ok(_) => {
-//XXX         tracing::info!("Data sent to followers: {}", data);
-//XXX     }
-//XXX     Err(e) => {
-//XXX         //TODO rediscover followers on comunication error
-//XXX         tracing::error!("Failed to send data to followers: {}", e);
-//XXX     }
-//XXX }
+match Self::append_entries_to_followers(vec![Entry { index: 0, data: data.clone() }], followers.clone()).await {
+    Ok(_) => {
+        tracing::info!("Data sent to followers: {}", data);
+    }
+    Err(e) => {
+        //TODO rediscover followers on comunication error
+        tracing::error!("Failed to send data to followers: {}", e);
+    }
+}
 
Suggestion importance[1-10]: 9

Why: Removing 'XXX' comments will indeed make the code cleaner and more professional. These comments are typically used for internal notes and should not be present in production code. This suggestion significantly improves code readability and maintainability.

9

@renancloudwalk renancloudwalk merged commit ab0b37b into main May 24, 2024
32 checks passed
@renancloudwalk renancloudwalk deleted the remove-append-from-consensus branch May 24, 2024 00:41
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.

1 participant