Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[zAdd] #26

Merged
merged 5 commits into from Oct 27, 2014
Merged

[zAdd] #26

merged 5 commits into from Oct 27, 2014

Conversation

sjeandeaux
Copy link
Contributor

zAdd with map with members has same score

  zAdd with map with members has same score
@curreli
Copy link
Contributor

curreli commented Oct 24, 2014

I'm not following. Could you please elaborate? What's the issue?

@curreli
Copy link
Contributor

curreli commented Oct 24, 2014

Oh right, I see what you mean. The map() method is performed on a Map, thus building a Map as a rIesult which discards duplicate scores.

I added a few comments, please review.

Good catch!

case class ZAdd[W](key: String, members: Map[W, scredis.Score])(
implicit writer: Writer[W]
) extends Request[Long](
) extends Request[Long](
Copy link
Contributor

Choose a reason for hiding this comment

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

I see an indentation issue here.

sjeandeaux and others added 4 commits October 24, 2014 13:04
  readable coe
  indention
  ide newline problem
@sjeandeaux
Copy link
Contributor Author

I reviewed my code and removed all difference not necessary.

Thank you for taking time to look my work

client.zRangeWithScores(
"SET"
).futureValue should contain theSameElementsInOrderAs List[(String, Score)](
("D", -2.6), ("C", -1.3), ("A", 2.5), ("B", 3.8)
("D", -2.6), ("E", -2.6), ("C", -1.3), ("A", 2.5), ("B", 3.8)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any guarantee from Redis that D will appear before E? If not then we should account for that in the test case and allow them to switch place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The elements having the same score are returned in lexicographical order.

http://redis.io/commands/zrangebyscore

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@curreli
Copy link
Contributor

curreli commented Oct 24, 2014

Looks good. I just have a small concern for the test case, please check my comment.

@curreli
Copy link
Contributor

curreli commented Oct 24, 2014

All good. Will include in the next minor release. Thank you for contributing.

curreli pushed a commit that referenced this pull request Oct 27, 2014
Fixed an issue in the implementation of the zAdd request which prevented multiple members with identical scores to be added.
@curreli curreli merged commit 9f749af into Livestream:master Oct 27, 2014
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.

None yet

2 participants