search

hsgubert / cassandra_migrations

Cassandra Migrations is a Cassandra database schema migration library for Rails applications.
MIT License
45 stars 43 forks source link

Refreshed gem #91

Open bitaxis opened 7 years ago

bitaxis commented 7 years ago

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.

hsgubert commented 6 years ago

Hi @bitaxis, interesting contributions. Is there anything you could contribute/update on the README file? Also, it seems to me the wiki page https://github.com/hsgubert/cassandra_migrations/wiki/Migrations-DSL could be updated with your new helpers.

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.

bitaxis commented 6 years ago

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.

hsgubert commented 6 years ago

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)

bitaxis commented 6 years ago

@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.

bitaxis commented 6 years ago

@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 announce_migration, announce_operation, and announce_suboperation. If you can provde some guidance/insight on how I might test them, I'd be happy to follow along.

Also, having reviewed the README.md file, I don't think I've changed anything that would cause existing features to break.

hsgubert commented 6 years ago

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:

  1. For the execute_text method, just pass a few examples of CQL text (including trailing spaces or not, semicolon or not) and check that execute is called with the expected parameter (you can use rspec-mocks for that)

  2. execute_statement: just test it delegates call to execute

  3. execute_file: stub File.read and make it return a CQL text when a certain argument is received. Then just check execute is called with the correct parameter

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.