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

Fix expiration and upppercase check #72

Merged

Conversation

gallir
Copy link
Contributor

@gallir gallir commented Jan 3, 2021

  • While constructing the frame, expiration is not checked: added a check and the additional bytes, i.e. PX ms.
  • When parsing from "EX" or "PX", it accepts only upper case, added to_uppercase() to the comparison.

@seanpianka
Copy link

I don't think bug fixes are in the spirit of this repository, as described by the README's warnings...

Copy link
Collaborator

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

@seanpianka Bug fixes and new features are not the same thing. Mini-redis should of course correctly implement the features it has, even if we don't accept new features.

src/cmd/set.rs Show resolved Hide resolved
@gallir
Copy link
Contributor Author

gallir commented Jan 3, 2021

I don't think bug fixes are in the spirit of this repository, as described by the README's warnings...

Which universe? It's a bug fix and from README.md:

## Contributing

Contributions to `mini-redis` are welcome...

src/cmd/set.rs Outdated
@@ -146,6 +146,11 @@ impl Set {
frame.push_bulk(Bytes::from("set".as_bytes()));
frame.push_bulk(Bytes::from(self.key.into_bytes()));
frame.push_bulk(self.value);
if let Some(ms) = self.expire {
// Add expiration in milliseconds: SET key value PX ms
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please elaborate this comment so it explains that px means milliseconds, and perhaps also that ex would mean seconds? Currently it is just an arbitrary string if the user is not familiar with this part of the redis protocol.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@seanpianka
Copy link

I don't think bug fixes are in the spirit of this repository, as described by the README's warnings...

Which universe? It's a bug fix and from README.md:


## Contributing



Contributions to `mini-redis` are welcome...

You've won this round.

@Darksonn Darksonn merged commit c3bc304 into tokio-rs:master Jan 4, 2021
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

3 participants