-
Notifications
You must be signed in to change notification settings - Fork 6
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
bubble sort #26
bubble sort #26
Conversation
This looks like a great start. Unfortunately, we also need a test for this. You should find a small test stub. |
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.
Thanks, looks good with the tests (covering some maybe interesting paths like []
).
For list inputs there may still be a subtlety about returning a copy or mutating the list in place.
Anyway, besides documentation, I think this is good!
def bubble_sort(l): | ||
# We should provide bubble sort as well! | ||
raise NotImplementedError | ||
def bubble_sort(arr): |
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.
This would need some documentation of course. At least for public facing functions.
# Test even-sized list: | ||
l = [3, 2, 1, 5, 4, 6] | ||
res = lws.bubble_sort(l) | ||
assert res == [1, 2, 3, 4, 5, 6] |
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.
This list of tests is good. One way one can write it more compact is for example pytest.mark.parametrize()
.
That way the input and expected results are parameter, and the repetitive code is a bit less (not much for the simple test). It also effectively splits it in multiple tests.
creating a new pull request with all the changes and closing this one |
You should be able to force-push, but OK :). |
just added bubble sort algo.