feat (facade): simple argument parser #1747

dranikpg · 2023-08-26T16:07:16Z

Adds a simple parser for handling command arguments

See comments below

dranikpg · 2023-08-26T16:16:27Z

With the search work I'm doing I need to parse a lot of parameters and the parsing code for it would grow over multiple hundreds of lines (given that I already have a few). Most of it is annoying boilerplate so I want to finally simplify it a little.

I want to add a proper argument parser instead of always looping manually, converting values, checking errors, etc. I want to use it for search and replace current mapping code in some command families.

Given that you'll be working on new commands as well with ACL, what do you think of it? What should be included?

I placed it in the redis_parser_test just because I was to lazy to add a separate target to compile some code, its just a playground for now.

My thoughts. Check this function out for example https://github.com/dranikpg/dragonfly/blob/search-named-return/src/server/search/search_family.cc#L97

What is an issue?

Iteration -> instead of manually doing i++ and keeping track of it, the parser should
Conversion -> instead of calling absl::SimpleAtoi the parser should
Its annoying to place if (i + 1 > args.size()) { cntx->Reply.... break/continue/etc} blocks, instead we can finish the block with default values and then check if any errors were reported before
The previous point is even more annoying with optional clauses where you can't calculate the size ahead.
Optional clauses (like field_name [AS ALIAS]) should be easy to handle with the parser
Cases where you would need to do a switch can be handled in one line as well

UPD: See the example in search family

royjacobson

I know this is still WIP, but still had a few comments to add :)

src/facade/arg_parser.h

kostasrim · 2023-08-28T12:51:49Z

I like the approach and it does simplify a lot of the bounds checking etc.

Given that you'll be working on new commands as well with ACL, what do you think of it? What should be included?

The ACL commands are very easy to parse, only acl setuser is a "beast". I can take care of any extensions when required so you don't have to worry about this.

P.s. I will take more detailed look later today.

royjacobson · 2023-08-29T08:16:23Z

src/facade/cmd_arg_parser.h

+
+  // Get optional error if occured
+  std::optional<ErrorInfo> Error() {
+    return std::move(error_);


safety suggestion:

Suggested change

return std::move(error_);

auto rv = std::move(error_);

error_.reset();

return rv;

then put CHECK(!error_.has_value()); in CmdArgParser's destructor

Today I learned that moving from an optional still keeps the value 😳

you made me double check 😆

a moved-from std::optional still contains a value, but the value itself is moved from.

I actually double checked as well 🤣

Today I learned that moving from an optional still keeps the value

It's up to the value_type of what the state of that value will be.

For the optional, has_value will return true. This is on par with the std::move semantics:

Unless otherwise specified, all standard library objects that have been moved from are placed in a "valid but unspecified state", meaning the object's class invariants hold (so functions without preconditions, such as the assignment operator, can be safely used on the object after it was moved from):

@dranikpg
try return std::exchange(error_, {}) for a slick one liner 👀

It's up to the value_type of what the state of that value will be.

I meant the state of the optional won't be nullopt

Signed-off-by: Vladislav Oleshko <[email protected]>

dranikpg · 2023-08-29T20:28:58Z

src/server/search/search_family.cc

    // RETURN {num} [{ident} AS {name}...]
-    if (ArgS(args, i) == "RETURN") {
-      if (i + 1 >= args.size()) {
-        (*cntx)->SendError(kSyntaxErr);
-        return nullopt;
-      }
-
-      uint64_t num_fields = args.size();
-      if (!absl::SimpleAtoi(ArgS(args, i + 1), &num_fields)) {
-        (*cntx)->SendError(kSyntaxErr);
-        return nullopt;
-      }
-
-      i += 1;
-
+    if (parser.Check("RETURN").ExpectTail(1)) {
+      size_t num_fields = parser.Next().Int<size_t>();
      alias_list = SearchParams::FieldAliasList{};
      while (alias_list->size() < num_fields) {
-        if (++i >= args.size()) {
-          (*cntx)->SendError(kSyntaxErr);
-          return nullopt;
-        }
-
-        string_view ident = ArgS(args, i);
-        string_view alias = ident;
-
-        if (i + 2 < args.size() && absl::EqualsIgnoreCase(ArgS(args, i + 1), "AS")) {
-          alias = ArgS(args, i + 2);
-          i += 2;
-        }
-
+        string_view ident = parser.Next();
+        string_view alias = parser.Check("AS").ExpectTail(1) ? parser.Next() : string_view{ident};
        alias_list->emplace_back(ident, alias);
      }


dranikpg · 2023-08-29T20:29:52Z

src/facade/cmd_arg_parser.h

+namespace facade {
+
+// Utility class for easily parsing command options from argument lists.
+struct CmdArgParser {


Is it actually ok to keep this header only?

ah, probably not (noticing 2s after giving the approval)

Signed-off-by: Vladislav Oleshko <[email protected]>

royjacobson

This is great :)

royjacobson

just for moving things into a .cc file

Signed-off-by: Vladislav Oleshko <[email protected]>

dranikpg · 2023-08-30T20:30:06Z

src/facade/cmd_arg_parser.cc

+
+#include <absl/strings/ascii.h>
+#include <absl/strings/match.h>
+#include <absl/strings/numbers.h>
+
+#include "base/logging.h"
+#include "facade/error.h"


I moved all function longer than 3 lines and all functions that have some sependencies (absl and our stuff)

src/facade/cmd_arg_parser.cc

src/facade/cmd_arg_parser.h

src/facade/cmd_arg_parser.cc

kostasrim · 2023-08-31T10:43:19Z

sorry for my late input 👨‍🍳

Great work, I left some comments 🚀

src/facade/cmd_arg_parser.cc

Signed-off-by: Vladislav Oleshko <[email protected]>

dranikpg force-pushed the arg-parser branch 2 times, most recently from 33fdeaa to 099969e Compare August 26, 2023 19:19

royjacobson reviewed Aug 27, 2023

View reviewed changes

dranikpg force-pushed the arg-parser branch 2 times, most recently from c432b1b to 4e52094 Compare August 27, 2023 20:42

dranikpg requested a review from royjacobson August 27, 2023 20:48

dranikpg mentioned this pull request Aug 28, 2023

feat(search): named return values #1746

Merged

royjacobson reviewed Aug 29, 2023

View reviewed changes

dranikpg added 6 commits August 29, 2023 23:18

feat: simple argument parser

3d521c4

Signed-off-by: Vladislav Oleshko <[email protected]>

chore: experiment v2

c688e4b

Signed-off-by: Vladislav Oleshko <[email protected]>

chore: experiment v3

7d65623

Signed-off-by: Vladislav Oleshko <[email protected]>

fix: rename arg_parser

87075df

fix: fixes v2

71dff73

Signed-off-by: Vladislav Oleshko <[email protected]>

chore: add cmd_arg_parser_test

e4d8581

Signed-off-by: Vladislav Oleshko <[email protected]>

dranikpg force-pushed the arg-parser branch from 4e52094 to f081f4f Compare August 29, 2023 20:22

dranikpg requested a review from royjacobson August 29, 2023 20:23

dranikpg commented Aug 29, 2023

View reviewed changes

fix: fix rebase

1a1a80c

Signed-off-by: Vladislav Oleshko <[email protected]>

dranikpg force-pushed the arg-parser branch from f081f4f to 1a1a80c Compare August 29, 2023 20:38

fix: small fixes

0a25ec3

Signed-off-by: Vladislav Oleshko <[email protected]>

dranikpg marked this pull request as ready for review August 30, 2023 06:20

royjacobson approved these changes Aug 30, 2023

View reviewed changes

royjacobson suggested changes Aug 30, 2023

View reviewed changes

fix: split into h/cc

6aa22a2

Signed-off-by: Vladislav Oleshko <[email protected]>

dranikpg commented Aug 30, 2023

View reviewed changes

dranikpg requested a review from royjacobson August 30, 2023 20:30

royjacobson previously approved these changes Aug 31, 2023

View reviewed changes

kostasrim reviewed Aug 31, 2023

View reviewed changes

src/facade/cmd_arg_parser.cc Show resolved Hide resolved

kostasrim reviewed Aug 31, 2023

View reviewed changes

src/facade/cmd_arg_parser.h Outdated Show resolved Hide resolved

src/facade/cmd_arg_parser.h Outdated Show resolved Hide resolved

src/facade/cmd_arg_parser.h Show resolved Hide resolved

kostasrim reviewed Aug 31, 2023

View reviewed changes

src/facade/cmd_arg_parser.cc Show resolved Hide resolved

kostasrim reviewed Aug 31, 2023

View reviewed changes

src/facade/cmd_arg_parser.cc Show resolved Hide resolved

fix: foxes

82468d1

Signed-off-by: Vladislav Oleshko <[email protected]>

dranikpg dismissed royjacobson’s stale review via 82468d1 August 31, 2023 14:11

dranikpg requested review from kostasrim and royjacobson August 31, 2023 14:14

kostasrim approved these changes Aug 31, 2023

View reviewed changes

dranikpg merged commit 47ef6d1 into dragonflydb:main Aug 31, 2023

dranikpg deleted the arg-parser branch September 15, 2023 06:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat (facade): simple argument parser #1747

feat (facade): simple argument parser #1747

dranikpg commented Aug 26, 2023 •

edited

Loading

dranikpg commented Aug 26, 2023 •

edited

Loading

royjacobson left a comment

kostasrim commented Aug 28, 2023

royjacobson Aug 29, 2023

dranikpg Aug 29, 2023

royjacobson Aug 30, 2023

kostasrim Aug 31, 2023 •

edited

Loading

dranikpg Aug 31, 2023

dranikpg Aug 29, 2023

dranikpg Aug 29, 2023

royjacobson Aug 30, 2023

royjacobson left a comment

royjacobson left a comment

dranikpg Aug 30, 2023 •

edited

Loading

kostasrim commented Aug 31, 2023

-    return std::move(error_);
+    auto rv = std::move(error_);
+    error_.reset();
+    return rv;

feat (facade): simple argument parser #1747

feat (facade): simple argument parser #1747

Conversation

dranikpg commented Aug 26, 2023 • edited Loading

dranikpg commented Aug 26, 2023 • edited Loading

royjacobson left a comment

Choose a reason for hiding this comment

kostasrim commented Aug 28, 2023

royjacobson Aug 29, 2023

Choose a reason for hiding this comment

dranikpg Aug 29, 2023

Choose a reason for hiding this comment

royjacobson Aug 30, 2023

Choose a reason for hiding this comment

kostasrim Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

dranikpg Aug 31, 2023

Choose a reason for hiding this comment

dranikpg Aug 29, 2023

Choose a reason for hiding this comment

dranikpg Aug 29, 2023

Choose a reason for hiding this comment

royjacobson Aug 30, 2023

Choose a reason for hiding this comment

royjacobson left a comment

Choose a reason for hiding this comment

royjacobson left a comment

Choose a reason for hiding this comment

dranikpg Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

kostasrim commented Aug 31, 2023

dranikpg commented Aug 26, 2023 •

edited

Loading

dranikpg commented Aug 26, 2023 •

edited

Loading

kostasrim Aug 31, 2023 •

edited

Loading

dranikpg Aug 30, 2023 •

edited

Loading