Request for comments on the two tests added #16

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@DeadZen
Contributor

DeadZen commented Feb 3, 2012

Uncomment the BREAK definition to see the errors.

@ericbmerritt

This comment has been minimized.

Show comment Hide comment
@ericbmerritt

ericbmerritt Feb 6, 2012

Owner

Deadzen,

I have been a bit swamped of late. So Judging by the commits I am assuming that these are a couple of unit tests. Would you mind going into some detail on the reason there are two files that are very nearly exactly the same? I am not a big fan of code duplication and I suspect once we know your purpose we can optimize this a bit.

Owner

ericbmerritt commented Feb 6, 2012

Deadzen,

I have been a bit swamped of late. So Judging by the commits I am assuming that these are a couple of unit tests. Would you mind going into some detail on the reason there are two files that are very nearly exactly the same? I am not a big fan of code duplication and I suspect once we know your purpose we can optimize this a bit.

@DeadZen

This comment has been minimized.

Show comment Hide comment
@DeadZen

DeadZen Feb 6, 2012

Contributor

Yes well this seems to be a bug present in both API's when checking siblings in relatives. Maybe somethings missing?
I'm also not a fan of code duplication, but this isn't for my purposes, its for the purpose of testing seresye examples, for which I found basically not a single one to be functional..
There appears to be two ways of using this library, (probably imported from ERESYE), in any event, there are obvious code misalignments, the previous patch fixes one of them..
The two relateapi tests are near exact copies on purpose to highlight ones I ran into..
If you're swamped, please look at it when you have the time..
The request for comments was not on the tests, or coding style advice but simply on the intended behaviour
of the code and perceived differences between otherwise seemingly exact copies...
The difference being only the API calls used are different and the fact that in neither case did it work.

The tests are designed to pass unless you uncomment the BREAK definition.

Contributor

DeadZen commented Feb 6, 2012

Yes well this seems to be a bug present in both API's when checking siblings in relatives. Maybe somethings missing?
I'm also not a fan of code duplication, but this isn't for my purposes, its for the purpose of testing seresye examples, for which I found basically not a single one to be functional..
There appears to be two ways of using this library, (probably imported from ERESYE), in any event, there are obvious code misalignments, the previous patch fixes one of them..
The two relateapi tests are near exact copies on purpose to highlight ones I ran into..
If you're swamped, please look at it when you have the time..
The request for comments was not on the tests, or coding style advice but simply on the intended behaviour
of the code and perceived differences between otherwise seemingly exact copies...
The difference being only the API calls used are different and the fact that in neither case did it work.

The tests are designed to pass unless you uncomment the BREAK definition.

@ericbmerritt

This comment has been minimized.

Show comment Hide comment
@ericbmerritt

ericbmerritt Feb 6, 2012

Owner

DeadZen,

I am not surprised the tests are not functional. Its an unreleased work in progress (WIP) and should be taken as such until it is released (even as an alpha). @yrashk has been promoting it on irc, which is all good, but he has not been letting people know that it is not complete, not even as an alpha. All that said I do still appreciate contributions.

Ok, with all of that out of the way you have cleared up a misconception on my part. I didn't realize this was pull request was mostly to illustrate brokenness in the code base not necessarily as an actual request to add code to the codebase. In light of that, I will probably take these, work up some illustrative tests and and commit those tests along with the fixes for them.

Again, I hope the tone of this did not come off poorly. I do very much appreciate finding and especially taking the time to illustrate bugs.

Owner

ericbmerritt commented Feb 6, 2012

DeadZen,

I am not surprised the tests are not functional. Its an unreleased work in progress (WIP) and should be taken as such until it is released (even as an alpha). @yrashk has been promoting it on irc, which is all good, but he has not been letting people know that it is not complete, not even as an alpha. All that said I do still appreciate contributions.

Ok, with all of that out of the way you have cleared up a misconception on my part. I didn't realize this was pull request was mostly to illustrate brokenness in the code base not necessarily as an actual request to add code to the codebase. In light of that, I will probably take these, work up some illustrative tests and and commit those tests along with the fixes for them.

Again, I hope the tone of this did not come off poorly. I do very much appreciate finding and especially taking the time to illustrate bugs.

@ericbmerritt

This comment has been minimized.

Show comment Hide comment
@ericbmerritt

ericbmerritt Feb 6, 2012

Owner

Rereading this comment to myself it comes off as pretty defensive. Which shouldn't be. Its code after all. The tests in the canonical repo should pass or the code shouldn't be in canonical regardless of the state of the project. Another area where working solo has made me a bit lazy. As an aside to the commits I will get that problem resolved as well.

Owner

ericbmerritt commented Feb 6, 2012

Rereading this comment to myself it comes off as pretty defensive. Which shouldn't be. Its code after all. The tests in the canonical repo should pass or the code shouldn't be in canonical regardless of the state of the project. Another area where working solo has made me a bit lazy. As an aside to the commits I will get that problem resolved as well.

@ericbmerritt

This comment has been minimized.

Show comment Hide comment
@ericbmerritt

ericbmerritt Jan 4, 2015

Owner

I am going to close this @DeadZen, We don't actually use this any more. I need to figure out a way to deprecate it.

Owner

ericbmerritt commented Jan 4, 2015

I am going to close this @DeadZen, We don't actually use this any more. I need to figure out a way to deprecate it.

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