Refreshed gem #91
Refreshed gem #91
Conversation
2. Lower Rails run-time dependency back down to 3.2
Refresh gem with updated dependencies, Ruby 2.4.1, and Rails 5.1 compatibility.
2. Invoke it with either execute_[sub]operation(cql) or execute_[sub]operation(message, cql).
2. Fine-tune #execute_[sub]operation.
2. Rename #execute_operation to #execute_statement. 3. Remove #execute_suboperation.
1. Change template to raise error by default. 2. Add helper methods.
|
Hi @bitaxis, interesting contributions. Just for consistency with previous contributions, could you add your name to the Contributors section on the README file instead of altering the .gemspec? In case you with to do bigger contributions and take the responsability to maintain the gem on the long term I would have no problem in adding you to the authors list in the future. |
|
Sure. I'll take a look at the Wiki pages and add my name to the list of contributors, and make improvements, should my use of it continues. However, I am not at a place where I would want to take over the gem's maintenance. |
|
no problem @bitaxis . I just mentioned about gem maintenance in case you wanted to be included in the authors list. Your contribution is very welcome in any case! (and in any size) |
2. Add !use_keyspace so that the migration needn't change with environments. 3. Improve !use by converting keyspace to string in case it's a symbol.
…r to work, for now. 2. Add spec for #use_keyspace.
2. Add additional keyspace environments for testing. 3. Add spec for .use_keyspace method. 4. Refine comments for the .use_keyspace method.
…a non-existent keyspace would result in an exception being raised.
|
@hsgubert, I updated the Wiki on your repo to match version 1.2.0 of the gem on my fork. However, I have since released version 1.3.1 of the gem on my fork, which contains functionality that does not exist in yours. Hence, I also cloned your Wiki and made changes there. Please take a look and satisfy this pull request as well as merging the Wiki changes I made back to yours. |
For now the best place to put your name and contact is on the README.md file as a contributor (also to be fair with other contributors). As I said earlier, if you wish to help maintain the gem on the long term I see no problem in putting you as an author too.
There was a problem hiding this comment.
You pull request is 90% ok. I just didn't find unit tests for the new methods in migration.rb.
Also, I know you improved the wiki pages, but please, do review the README.md file to see if all instructions there still work and the versions of dependencies are still correct. I noticed you had forgotten to put yourself as a contributor there, so I am not so confident you reviewed the README.md file.
Address those issues and we can merge your commits and publish a new version of the gem.
Thank you for your help in this project.
|
@hsgubert, thank you for acknowledging my contributions. I took a look at the existing specs, and I don't see tests for previously existing methods such as Also, having reviewed the |
|
I see your point. I think the reason why we never were really strict about the announce_ methods is because they just print statements. But it wouldn't hurt to have tests for them as well, just to ensure no crash happens when they're called. About the new execute_ methods you created, they have business logic in them and therefore should be tested. We just need 3 test cases:
Something like that is enough. If you want to create tests for the announce_ methods you can too, but I'm ok with keeping them without unit tests. |
I refreshed the gem with updated dependency versions, added Rails 5.0 & 5.1 to the appraisal, and use Ruby 2.4.1 as well.
All tests and appraisals pass.