-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Refactor ChangeFeedCommand.actor.cpp for Readability and Modularization #11461
base: main
Are you sure you want to change the base?
Conversation
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.
This looks great! thanks for the refactor.
fdbcli/ChangeFeedCommand.actor.cpp
Outdated
} | ||
throw; | ||
} | ||
return handleStreamCommand(localDb, tokens, warn); |
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.
It might be great if a const std::unordered_map<std::string_view, decltype(some_general_handle_function_type)>
could help defeat this if/elseif/else
monster.
foundationdb.sln
Outdated
@@ -0,0 +1,62 @@ | |||
| |||
Microsoft Visual Studio Solution File, Format Version 12.00 |
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.
Not sure if we want this SLN file since on our side we do not support MS windows -- it is supported by some other people I do not know. @ammolitor might know how to contact them.
fdbcli/ChangeFeedCommand.actor.cpp
Outdated
} else { | ||
range = KeyRangeRef(tokens[3], tokens[4]); | ||
} | ||
const CommandMap commandHandlers = { |
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.
Not sure what happened, but I just commented and they are not showing up. So forgive me if this commit is commented twice.
Can we move this outside the ACTOR
? So this is not being constructed repeatly when the function is called..
fdbcli/ChangeFeedCommand.actor.cpp
Outdated
} else { | ||
printUsage(tokens[0]); | ||
return false; | ||
return Future<bool>(false); |
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.
This cast is not necessary since Future(const T&)
is not defined as explicit
.
@@ -146,7 +146,40 @@ ACTOR Future<bool> handlePopCommand(Database localDb, std::vector<StringRef> tok | |||
namespace fdb_cli { | |||
|
|||
using CommandHandler = Future<bool> (*)(Database localDb, std::vector<StringRef> tokens, Future<Void> warn); |
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.
Not quite sure if a std::function would be better than a raw function pointer, but anyway.
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
FYI: I have sent you a pull request to your repo for the source formatting issue. |
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.
Um... This definitively would not pass the CI test because we are using rather than 4 spaces. I have no idea why but that's the rule.
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Modularized Code*: Separated command handling into individual functions (
handleStreamCommand
,handlePopCommand
).2. Consistent Error Handling: Improved error handling across functions with detailed error messages.
3. Enhanced Readability: Added comments and improved code structure for better readability.
4. Namespace Usage: Encapsulated helper functions within an unnamed namespace for better encapsulation.