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

chore: remove unused flush function #1237

Merged
merged 2 commits into from
Jun 25, 2024
Merged

chore: remove unused flush function #1237

merged 2 commits into from
Jun 25, 2024

Conversation

dinhani-cw
Copy link
Contributor

No description provided.

@dinhani-cw dinhani-cw enabled auto-merge (squash) June 25, 2024 18:21
Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5] 1
🧪 Relevant tests No
🔒 Security concerns No
⚡ Key issues to review None

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Maintainability
Add a transitional method or deprecation warning for the removed flush method

Ensure that the removal of the flush method does not impact other components that might
rely on this behavior. Consider adding a deprecation warning or a transitional method that
logs a message or throws an error if the removal of flush leads to issues in the system.

src/eth/storage/temporary_storage.rs [55]

 fn reset(&self) -> anyhow::Result<()>;
+// Transitional method to handle removal of flush
+#[deprecated(note = "flush method is deprecated and will be removed in future releases")]
+fn flush(&self) -> anyhow::Result<()> {
+    log::warn("flush method is deprecated and will be removed.");
+    Ok(())
+}
 
Suggestion importance[1-10]: 9

Why: Adding a deprecation warning or a transitional method for the removed flush method is important to ensure that other components relying on this method are not affected. This helps maintain backward compatibility and informs developers of the change.

9
Enhancement
Add a debug log statement to confirm state reset

Consider implementing a logging or debug statement to confirm the successful reset of the
state. This can be helpful for debugging and ensuring that the reset operation is
performed as expected.

src/eth/storage/inmemory/inmemory_temporary.rs [239-240]

 state.head.reset();
+log::debug("State has been successfully reset.");
 Ok(())
 
Suggestion importance[1-10]: 7

Why: Adding a debug log statement can be useful for debugging and ensuring that the reset operation is performed as expected. However, it is not crucial for the functionality of the code.

7

@dinhani-cw dinhani-cw merged commit 161e710 into main Jun 25, 2024
29 checks passed
@dinhani-cw dinhani-cw deleted the remove-flush branch June 25, 2024 18:28
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