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

Relax R version dependency #3

Merged
merged 1 commit into from Nov 7, 2017

Conversation

Projects
None yet
2 participants
@jonmcalder
Contributor

jonmcalder commented Sep 14, 2017

I'm not sure yet what the actual minimum requirements for CastleOfR are, but since swirl uses R (>=3.1.0) and you're using a similar callback strategy I assume this should be a safe minimum version for now?

Relax R version dependency
I'm not sure yet what the actual minimum requirements for CastleOfR are, but since swirl uses R (>=3.1.0) and you're using a similar callback strategy I assume this should be a safe minimum version for now?
@gsimchoni

This comment has been minimized.

Show comment
Hide comment
@gsimchoni

gsimchoni Sep 15, 2017

Owner

Thank you Jon, but I still would like to test it before merging (I agree, there's nothing here that even 2.1 didn't have, as far as I remember).

Owner

gsimchoni commented Sep 15, 2017

Thank you Jon, but I still would like to test it before merging (I agree, there's nothing here that even 2.1 didn't have, as far as I remember).

@jonmcalder

This comment has been minimized.

Show comment
Hide comment
@jonmcalder

jonmcalder Sep 15, 2017

Contributor

Ok sure.

With regards to testing, I started working through the code last night and looked at adding some tests, but as you say in #2 it's not easy to test this as it is currently. Hopefully in some places we can look at breaking up some of the code into more testable functions, but that leaves us with a bit of a chicken and egg scenario i.e. refactoring without testing may break functionality, but it's probably not possible to test much without first refactoring. I guess there'll just need to be a lot of manual testing initially... you know best since you wrote it 😄

Contributor

jonmcalder commented Sep 15, 2017

Ok sure.

With regards to testing, I started working through the code last night and looked at adding some tests, but as you say in #2 it's not easy to test this as it is currently. Hopefully in some places we can look at breaking up some of the code into more testable functions, but that leaves us with a bit of a chicken and egg scenario i.e. refactoring without testing may break functionality, but it's probably not possible to test much without first refactoring. I guess there'll just need to be a lot of manual testing initially... you know best since you wrote it 😄

@gsimchoni gsimchoni merged commit 9494755 into gsimchoni:master Nov 7, 2017

@gsimchoni

This comment has been minimized.

Show comment
Hide comment
@gsimchoni

gsimchoni Nov 7, 2017

Owner

Deeply sorry it took so long, Jon.

Owner

gsimchoni commented Nov 7, 2017

Deeply sorry it took so long, Jon.

@jonmcalder

This comment has been minimized.

Show comment
Hide comment
@jonmcalder

jonmcalder Nov 7, 2017

Contributor

No worries!

Contributor

jonmcalder commented Nov 7, 2017

No worries!

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