Skip to content

MOD-5606: Rename FORMAT args and change defaults#1056

Merged
oshadmi merged 1 commit intomasterfrom
oshadmi_format_args_changes
Jul 27, 2023
Merged

MOD-5606: Rename FORMAT args and change defaults#1056
oshadmi merged 1 commit intomasterfrom
oshadmi_format_args_changes

Conversation

@oshadmi
Copy link
Collaborator

@oshadmi oshadmi commented Jul 27, 2023

  • JSON.GET
    Rename JSON to EXPAND1
    Default is STRING

  • JSON.ARRPOP
    Rename STRING to STRINGS
    Rename JSON to EXPAND1
    Default is STRINGS

Followup PR:
Support FORMAT STRINGS in JSON.GET to return an array of arrays of STRING
(as with JSON.ARRPOP)

@oshadmi oshadmi requested a review from MeirShpilraien July 27, 2023 05:55
@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Patch coverage: 96.42% and project coverage change: +0.12% 🎉

Comparison is base (89f1f1b) 78.97% compared to head (dd0c55f) 79.09%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1056      +/-   ##
==========================================
+ Coverage   78.97%   79.09%   +0.12%     
==========================================
  Files          15       15              
  Lines        3820     3842      +22     
==========================================
+ Hits         3017     3039      +22     
  Misses        803      803              
Files Changed Coverage Δ
redis_json/src/c_api.rs 7.83% <0.00%> (ø)
redis_json/src/ivalue_manager.rs 92.24% <ø> (+0.18%) ⬆️
redis_json/src/commands.rs 97.08% <100.00%> (-0.07%) ⬇️
redis_json/src/formatter.rs 95.72% <100.00%> (-0.08%) ⬇️
redis_json/src/key_value.rs 93.78% <100.00%> (+0.41%) ⬆️
redis_json/src/redisjson.rs 83.33% <100.00%> (+0.26%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +121 to +125
let next = args.next_str()?;
if next.eq_ignore_ascii_case("STRINGS") {
return Err(RedisError::Str("ERR wrong reply format"));
}
format_options.format = ReplyFormat::from_str(next)?;

Choose a reason for hiding this comment

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

Why not comparing the format and avoid another string comparison ?

if next.eq_ignore_ascii_case("STRING") {
return Err(RedisError::Str("ERR wrong reply format"));
}
format_options.format = ReplyFormat::from_str(next)?;

Choose a reason for hiding this comment

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

Same as above...

Copy link

@MeirShpilraien MeirShpilraien left a comment

Choose a reason for hiding this comment

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

Only approving the rust code. I did not take into consideration any of the API changes or possibly breaking changes...

@oshadmi oshadmi merged commit de5999b into master Jul 27, 2023
@oshadmi oshadmi deleted the oshadmi_format_args_changes branch July 27, 2023 09:42
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.

2 participants