Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support JSON.MERGE Command #132

Merged
merged 35 commits into from
May 14, 2023
Merged

Support JSON.MERGE Command #132

merged 35 commits into from
May 14, 2023

Conversation

shacharPash
Copy link
Contributor

@shacharPash shacharPash commented May 9, 2023

Closes #129

@shacharPash shacharPash temporarily deployed to REDIS_USER May 9, 2023 09:51 — with GitHub Actions Inactive
@shacharPash shacharPash temporarily deployed to REDIS_USER May 9, 2023 09:51 — with GitHub Actions Inactive
@shacharPash shacharPash temporarily deployed to REDIS_USER May 9, 2023 09:51 — with GitHub Actions Inactive
@shacharPash shacharPash temporarily deployed to REDIS_USER May 9, 2023 09:51 — with GitHub Actions Inactive
@codecov-commenter
Copy link

codecov-commenter commented May 9, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (524dfb0) 93.23% compared to head (8279820) 93.25%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
+ Coverage   93.23%   93.25%   +0.02%     
==========================================
  Files          78       78              
  Lines        4611     4625      +14     
  Branches      427      426       -1     
==========================================
+ Hits         4299     4313      +14     
  Misses        189      189              
  Partials      123      123              
Impacted Files Coverage Δ
src/NRedisStack/Json/DataTypes/KeyValuePath.cs 78.57% <100.00%> (-3.79%) ⬇️
src/NRedisStack/Json/JsonCommandBuilder.cs 91.87% <100.00%> (+0.15%) ⬆️
src/NRedisStack/Json/JsonCommands.cs 88.27% <100.00%> (+0.59%) ⬆️
src/NRedisStack/Json/JsonCommandsAsync.cs 88.27% <100.00%> (+0.59%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@shacharPash shacharPash marked this pull request as ready for review May 10, 2023 13:49
@shacharPash shacharPash requested review from slorello89 and chayim May 10, 2023 14:43
@shacharPash shacharPash requested a review from a team May 10, 2023 14:50
/// <param name="json">The value to set.</param>
/// <returns>The disposition of the command</returns>
/// <remarks><seealso href="https://redis.io/commands/json.merge"/></remarks>
bool Merge(RedisKey key, RedisValue path, RedisValue json);
Copy link
Contributor

Choose a reason for hiding this comment

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

So clean!

Copy link
Member

@slorello89 slorello89 left a comment

Choose a reason for hiding this comment

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

Looks like there's a breaking change in here (public class renamed) - so if that's right then make sure that this is marked as a break, if not revert. Also I think the docs on the merge command are just bad as they are a straight copy/pasta of JSON.SET, when that's not quite accurate, probably need them to update what's in their commands.json but the merge command will merge the JSON value with the JSON value in the key to create a composite containing all the keys both object's. Really need more clarity behind how it works, but the way it's currently marked here isn't quite right (even if the argument list is fine and the code should work)

@@ -2,22 +2,17 @@

namespace NRedisStack.Json.DataTypes;

public struct KeyValuePath
public struct KeyPathValue
Copy link
Member

Choose a reason for hiding this comment

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

this is a breaking change, so if this is right, make sure to apply the label and put it in the notes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this struct merged yesterday

bool MSet(KeyPathValue[] KeyPathValueList);

/// <summary>
/// Sets or updates the JSON value at a path.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is accurate?

@chayim chayim merged commit 7d2b006 into master May 14, 2023
@chayim chayim deleted the Issue129/JSON.MERGE branch May 14, 2023 11:01
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for JSON.MERGE
4 participants