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

Support Cassandra versions after 2.0.x #22

Merged

Conversation

jancona
Copy link
Contributor

@jancona jancona commented Dec 21, 2015

I took a stab at a fix for #21. With this commit, the tests pass with 2.0, 2.1, 2.2 and 3.0. I also rev'd the version used in the tests to 2.2.4.

@edwardcapriolo
Copy link
Owner

This is great. I do not mind merging. However I have a suggesting. Rather than try { } catch. We could make construct farsandra with a version and based on the version we do different replace logic?

Something like:

interface farsandra {
createBase();
}
class farsandra2.2 {
createBase(){
replaceXWithY()
}
}

class farsandra2.1 {
createBase(){
replacePWithQ()
}
}

Usage would be:
farsandra f = FarFactory.create("2.2");

I think if we do this over time it will be cleaner to manage all the differences. If your interested in moving forward with this let me know. Otherwise I will merge this and I can work on that later!

@jancona
Copy link
Contributor Author

jancona commented Dec 22, 2015

I don't particularly like the try/catch approach, but I'm afraid that a class per version approach would lead to a lot of duplicated code. I'll take a look to see if I can find a clever way to factor it.
The other downside is that replacing the constructors with factory methods won't be backwards compatible.

@edwardcapriolo
Copy link
Owner

{quote}
I'll take a look to see if I can find a clever way to factor it.
The other downside is that replacing the constructors with factory methods won't be backwards compatible.
{quote}

You are right about this. Obviously it is annoying for a user of any library to have to deal with breaking changes, however we also dont want farsandra to be a series of try catch and conditionals. imagine if the code base has to span 4 versions and the replacement logic is ugly and hard to follow. Again if you can not come up with a solution there you are happy with just punt on it and I will commit what you have.

@jancona
Copy link
Contributor Author

jancona commented Dec 22, 2015

I tried a couple of refactorings that seemed to make things worse rather than better. I could replace the try/catch's with case statements or if/then's, if you think that would be cleaner.

One other thought: perhaps Farsandra should package the modified config files, rather than attempting to edit them on the fly.

In any case, I'd like to see some version of this PR merged before trying anything bigger. I can try to remove the try/catch's first, if you'd like.

@edwardcapriolo
Copy link
Owner

I will look this over and merge it today.

edwardcapriolo added a commit that referenced this pull request Dec 27, 2015
@edwardcapriolo edwardcapriolo merged commit e7f99e3 into edwardcapriolo:master Dec 27, 2015
@edwardcapriolo
Copy link
Owner

This is merged. I will look for a better way to support multiple versions in the future.

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.

2 participants