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

Enhancement: Implement bubble sort #30

Closed

Conversation

andrea-petrocchi
Copy link
Contributor

Fix #9.

Copy link
Collaborator

@seberg seberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you this looks great. Made sure to have a test and all boxes are ticked :). In bigger projects/more complex, you would still have to expect some iteration/fixups, unfortunately.

But if I see a PR with this care, I would be very happy! :).

# We should provide bubble sort as well!
raise NotImplementedError
"""Bubble sort algorithm.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bigger project might want more documentation, but this is great!

@seberg
Copy link
Collaborator

seberg commented Aug 29, 2024

@andrea-petrocchi, sorry that I will close this now without merging. Unfortunately, I looked at the other PR first, which collided. (And of course you even commented on the issue!)
If this was a bigger project, we should probably made sure that you do get some ackowledgement (e.g. a at least "Co-authored-by") so that your work is not lost.

@seberg seberg closed this Aug 29, 2024
@andrea-petrocchi
Copy link
Contributor Author

@seberg thank you for your comments. I also saw the other PR being completed before mine. In cases like this is it good practice to close the PR myself or to let the reviewer do that?

@seberg
Copy link
Collaborator

seberg commented Aug 30, 2024

I think it is great if you just take initiative in such cases.

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.

ENH: Implement bubble sort
2 participants