Split commands into different files & add bitcount #78

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

First, I split all the commands into different files. Each file match a redis commands category. Also, I add comments to know for each category which commands are implemented and which are not (redis version 2.8.2).

The implemented methods have the same name as the corresponding command, but when you want to edit that file it can be a little messy, so I thought that would be ok to organize the files in that way. Hope you like it.

Now bitcount is implemented (specs added too). It would be great if you can take a look on it.

Things can be improved:

  • Docs. I know the methods are clear because its name, but I think YARD or RDOC documentation would be a good added value to the gem.
  • Decouple into more modules.
  • Use concerns to handle dependencies
  • Constants. Everywhere is used 'OK'
  • Exceptions for unimplemented methods (when I have the problem with bitcount it wasn't obvious for me to think that the gem doesn't implement that command). Add those exceptions also avoid the documentation to know which methods are not implemented (still it can be kept as a summary) and the placeholders acts as a good help for developers.
  • Group specs. I do that for bitcount, if you like the example it can be used as an example. Is hard to read which methods have specs.

Sorry about english, and thanks for the gem.
Regards

Collaborator

caius commented Dec 4, 2013

Hey @ivanetchart,

Thank you for your contribution, but could you possible write the PR description in english please? I'm afraid I don't speak spanish, and in the open source world english is considered the main language to communicate in unless a project specifies otherwise. The code diff looks interesting, but I can't tell your intent of it from the description I'm afraid.

Thank you!

Sure, I'm afraid I don't speak english very well, but I will try.

PD: @caius I understand what do you say, and that's why commit name, and commented code is in english. Because that is what will be shared. Not this pull request comments. I thought that people who would check this pull request speak spanish (case of @guilleiguaran), but obviously I missed something about the process, my mistake. Thanks for your answer!

Collaborator

caius commented Jan 16, 2014

Hey @ivanetchart, thanks for translating the description. It's certainly a good read! I thank you in advance for taking the time to put this together.

I'm somewhat hesitant to merge this in one go though, it's a little too large for me to comfortably get a grasp on the changes. 😢 Would it be possible to get this broken down into a few pull requests instead please? 🔨 Perhaps one PR for grouping the commands into modules & adding docs to the (unchanged) methods. Then another PR on top of that to gather up commonly used strings into Constants; and another for unimplemented methods causing an error.

Makes it somewhat easier to see and judge each change on it's own merit and comment on the specific changes if needed, rather than this large pull request changing many things at once. I hope you understand, and I'd love to get this merged in, you've got some good contributions & ideas in here from reading the description. ❤️🌟

Owner

guilleiguaran commented Feb 6, 2014

@ivanetchart can you send the bitcount implementation in a separated PR?

I would like to get this merged but as @caius said it's a big change to do in a single commit 😁

Let me know if you want some help on this (or want to pair doing this) on this to get this merged soon 😄

@caius @guilleiguaran I can do that later today. Thanks for your comments.

Collaborator

caius commented May 20, 2014

Going to close this for now.

@ivanetchart Please feel free to open a new pull request for the bitcount work, and further PRs for any other tidy up work!

caius closed this May 20, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment