-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add tests for Line #33
base: master
Are you sure you want to change the base?
Conversation
tests/tests_euclid/test_line.py
Outdated
(Line(0, 0, 0, 0), Line(0, 0, 0, 0), True), | ||
(Line(0, 0, 0, 1), Line(1, 0, 1, 0), False), | ||
(Line(0, 0, 1, 1), Line(0, 1, 1, 0), False), | ||
(Line(0, 0, 1, 0), Line(1, 0, 1, 0), True), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several of the lines here are I'll defined. The two points defining some lines are coincident so there is no well-defined meaning of "parallel" and you can see the implementation returns True (1st and 4th) for some and False (2rd) for others. Can you make all the lines here non-zero length?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey,
oh yes I see your point :-)
Some remarks:
The two points defining some lines are coincident so there is no well-defined meaning of "parallel" and you can see the implementation returns True (1st and 4th)
Of course there is no mathematical definition of "parallel", but here the problem is that python doesn't know that a Line
with start point and end point equals is just a Point
. Perhaps an idea is to think to threat this case in the initialisation of a Line
, i.e., if start_point = end_point
, then return a Point
-class instead of a Line
-class
Can you make all the lines here non-zero length?
I added better test cases ;-). However, tests are made to test the method. In this case, we saw that there is this problem with the Line
-class when taking start point and end point equal. In conclusion, non-zero length Lines are also good test cases :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if start_point = end_point, then return a Point-class instead of a Line-class
A point is very different than an ill-defined line. This would not be type-stable, where the type of the result depends on the particular values of the arguments, which makes code much more error-prone and is generally bad practice.
In conclusion, [sic.] zero length Lines are also good test cases :-)
I agree zero-length (that's what you mean to type, right?) lines are a corner case that we should now test. I did not consider zero-length lines when writing the code originally so do not assume that my implementation parallel_to()
returns a sensible answer. Instead of writing a test that agrees with the current implementation of parallel_to()
, let us define the expected behavior. If this differs from the current implementation, then we found a bug!
My initial thoughts are that parallel_to()
should be consistent no matter the position of a line. That means all "zero-length lines" are equivalent and should either be "parallel to" all other lines or "parallel to" no other lines. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A point is very different than an ill-defined line. This would not be type-stable, where the type of the result depends on the particular values of the arguments, which makes code much more error-prone and is generally bad practice.
I see your point.
Perhaps we should define, or make clear, what a Line is.
By Euclide, a Line is "breadthless length that lies evenly with respect to the points on itself".
We have now two cases
- if length > 0, then we should thrown an error if
start_point
andend_point
are equal. In this case you want to define a point not a line; - if length >= 0, then we should really take care of the case of zero-length line (as you mentioned for the method
parallel_to()
My initial thoughts are that parallel_to() should be consistent no matter the position of a line. That means all "zero-length lines" are equivalent and should either be "parallel to" all other lines or "parallel to" no other lines. What do you think?
I don't have strong opinion. But I will suggest that we define a line to have non zero length. In this way, we don't have this kind of situation like here and things are clear. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey, @cduck just bumping the PR ;-)
add test cases
hey @cduck just bumping the PR :-) |
hey @cduck :-) |
No description provided.