Skip to content

Conversation

jsoref
Copy link

@jsoref jsoref commented Jun 4, 2025

No description provided.

jsoref added 30 commits June 3, 2025 23:01
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
jsoref added 3 commits June 3, 2025 23:12
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
MODULE LOAD /path/to/libjson.so
```
## Supported Module Commands
## Supported Module Commands
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't usually make commits to drop spaces... this one seems like a pretty good candidate for dropping...

#define ERRMSG_JSON_DOCUMENT_NOT_FOUND "NONEXISTENT JSON document is not found"
#define ERRMSG_NEW_VALKEY_KEY_PATH_NOT_ROOT "SYNTAXERR A new Valkey key's path must be root"
#define ERRMSG_CANNOT_DISABLE_MODULE_DUE_TO_OUTSTADING_DATA \
#define ERRMSG_CANNOT_DISABLE_MODULE_DUE_TO_OUTSTANDING_DATA \
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hoping this is private

JSONUTIL_EMPTY_JSON_OBJECT,
JSONUTIL_EMPTY_JSON_ARRAY,
JSONUTIL_INDEX_OUT_OF_ARRAY_BOUNDARIES,
JSONUTIL_INDEX_OUT_OF_ARRAY_BOUNDS,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hoping this is private enough... otherwise, I'm happy to drop this or any other change...

\brief Macro to indicate a parse error.
\param parseErrorCode \ref rapidjson::ParseErrorCode of the error
\param offset position of the error in JSON input (\c size_t)
\param offset position of the error in JSON input (\c size_t)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there were a couple of places where things appeared to be intentionally indented. This doesn't appear to be one of those cases...

}

// When read/write pointers are the same for insitu stream, just skip unescaped characters
// When read/write pointers are the same for in situ stream, just skip unescaped characters
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thi is the proper spelling for the phrase.

https://en.wikipedia.org/wiki/In_situ

Note that I'm not changingt the class as that seemed a bit too invasive...

Comment on lines -18 to +25
def is_outofboundaries_error(string):
return string.startswith("OUTOFBOUNDARIES")
def is_outofbounds_error(string):
return string.startswith("OUTOFBOUNDS")

def is_limit_exceeded_error(string):
return string.startswith("LIMIT")

def is_write_error(string):
return string.startswith("ERROR") or string.startswith("OUTOFBOUNDARIES") or \
return string.startswith("ERROR") or string.startswith("OUTOFBOUNDS") or \
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes indicate that I'm changing something of an API. If that's a problem, I can drop the changes...

assert exp.encode() == client.execute_command('JSON.GET', k2, path)

# we do not support mixing of unions and slices, nor do we support extraneous commas
# we do not support mixing of unions and slices, nor do we support extraneous commas
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is why I included the commit to fix the doubled whitespace.

actually, this line helped me realize that one of my own rules was broken -- one of the benefits I get from running my rules against other projects is identifying bugs in them and fixing them...

'JSON.GET', key, path).decode()

def test_json_objectlen_command(self):
def test_json_object_len_command(self):
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the actual things are objlen or objkeys, so I don't see any particular reason to invent additional new words just for these tests...

}

TEST_F(DomTest, testNumMutiBy_int64) {
TEST_F(DomTest, testNumMultiBy_int64) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd personally favor Multiply, but this was the simplest fix

" \"my key\" : \"key inside here\""
" },"
" \"anonther object\" : {"
" \"another object\" : {"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub CI passes...

Copy link
Member

@roshkhatri roshkhatri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Over all looks good, just need change a few things. I would also like to see why the tests run. I am curious they didnt run on this


//
// Setup the global string table
// Set up the global string table
Copy link
Member

@roshkhatri roshkhatri Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Set up the global string table
// Set up the global string table

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setup is a noun, this is trying to use a verb. I'm happy to drop as required, but it's wrong.

Copy link
Member

@roshkhatri roshkhatri Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay lets keep it

Co-authored-by: Roshan Khatri <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
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.

3 participants