torwig commented on code in PR #1032:
URL:
https://github.com/apache/incubator-kvrocks/pull/1032#discussion_r1002747022
##########
tests/gocase/unit/type/strings/strings_test.go:
##########
@@ -605,12 +605,12 @@ func TestString(t *testing.T) {
})
t.Run("Extended SET with incorrect expire value", func(t *testing.T) {
- require.ErrorContains(t, rdb.Do(ctx, "SET", "foo", "bar", "ex",
"1234xyz").Err(), "not an integer")
- require.ErrorContains(t, rdb.Do(ctx, "SET", "foo", "bar", "ex",
"0").Err(), "invalid expire time")
- require.ErrorContains(t, rdb.Do(ctx, "SET", "foo", "bar",
"exat", "1234xyz").Err(), "not an integer")
- require.ErrorContains(t, rdb.Do(ctx, "SET", "foo", "bar",
"exat", "0").Err(), "invalid expire time")
- require.ErrorContains(t, rdb.Do(ctx, "SET", "foo", "bar",
"pxat", "1234xyz").Err(), "not an integer")
- require.ErrorContains(t, rdb.Do(ctx, "SET", "foo", "bar",
"pxat", "0").Err(), "invalid expire time")
+ require.ErrorContains(t, rdb.Do(ctx, "SET", "foo", "bar", "ex",
"1234xyz").Err(), "non-integer")
Review Comment:
@PragmaTwice Really nice job!
These error messages (and others like `ERR wrong number of arguments`) were
written that way to be consistent with the `Redis` protocol.
```
127.0.0.1:6379> set foo bar ex 1234tyg
(error) ERR value is not an integer or out of range
127.0.0.1:6379> set foo bar ex 0
(error) ERR invalid expire time in 'set' command
127.0.0.1:6379>
```
So I'm not sure if it's correct to change them.
##########
src/commands/command_parser.h:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <algorithm>
+#include <cctype>
+#include <functional>
+#include <iterator>
+
+#include "parse_util.h"
+#include "status.h"
+#include "util.h"
+
+template <typename Iter>
+struct MoveIterator : Iter {
+ explicit MoveIterator(Iter iter) : Iter(iter){};
+
+ using Iter::value_type;
+
+ typename Iter::value_type&& operator*() const { return
std::move(this->Iter::operator*()); }
+};
+
+template <typename Iter>
+struct CommandParser {
+ public:
+ using value_type = typename Iter::value_type;
+
+ CommandParser(Iter begin, Iter end) : begin(begin), end(end) {}
+
+ template <typename Container>
+ explicit CommandParser(const Container& con, size_t skip_num = 0)
+ : begin(std::begin(con) + skip_num), end(std::end(con)) {}
+
+ template <typename Container>
+ explicit CommandParser(Container&& con, size_t skip_num = 0)
+ : begin(MoveIterator(std::begin(con) + skip_num)),
end(MoveIterator(std::end(con))) {}
+
+ decltype(auto) RawPeek() const { return *begin; }
+
+ decltype(auto) RawTake() { return *begin++; }
+
+ decltype(auto) RawNext() { ++begin; }
+
+ bool Good() const { return begin != end; }
+
+ template <typename Pred>
+ bool EatPred(Pred&& pred) {
+ if (Good() && std::forward<Pred>(pred)(RawPeek())) {
+ RawNext();
+ return true;
+ } else {
+ return false;
+ }
+ }
+
+ bool EatEqICase(std::string_view str) {
+ return EatPred([str](const auto& v) { return Util::EqualICase(str, v); });
Review Comment:
In some places, you have `&` near the type but in the `kvrocks` it's usually
used near the argument's name. It's just about the style consistency across the
project.
##########
src/commands/redis_cmd.cc:
##########
@@ -393,15 +358,18 @@ class CommandGet : public Commander {
class CommandGetEx : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- white_list_ = {{"persist", false}};
- auto s = ParseTTL(std::vector<std::string>(args.begin() + 2, args.end()),
&white_list_, &ttl_);
- if (!s.IsOK()) {
- return s;
- }
- if (white_list_["persist"] && args.size() > 3) {
- return Status(Status::NotOK, errInvalidSyntax);
+ CommandParser parser(args, 2);
+ std::string_view ttl_flag;
+ while (parser.Good()) {
+ if (auto v = GET_OR_RET(ParseTTL(parser, ttl_flag))) {
+ ttl_ = *v;
+ } else if (parser.EatEqICaseFlag("PERSIST", ttl_flag)) {
+ persist_ = true;
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- return Commander::Parse(args);
+ return {};
Review Comment:
Personally, I'd like to see more verbose but explicit `Status::OK()` instead
of `{}`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
