Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

unit-test to catch a TimSort issue #22

Closed
wants to merge 1 commit into from
Closed

Conversation

duzun
Copy link

@duzun duzun commented Feb 14, 2020

Description

This is a failing test to prove TimSort is badly implemented.

Motivation and Context

Current implementation of TimSort works for arrays up to 32 items only.

How Has This Been Tested?

I've just generated a list of 42 numbers, from 1 to 42, shuffled it and sorted it with TimSort. The result should equal the initial sorted array.

Current implementation of TimSort works for arrays up to 32 items only.
@doganoo
Copy link
Owner

doganoo commented Feb 15, 2020

Hey,

thank you for this PR.

I will check this.

Do you also have a suggestion for the TimSort implementation?

EDIT

could you also please add your name and email to the sources you have changed?

@doganoo
Copy link
Owner

doganoo commented Feb 27, 2020

again, thank you for this PR.

Unfortunately I will not merge this branch into master since we do not want to prove broken code. You are very welcome to contribute to the TimSort class in order to fix whatever you think is broken (including the corresponding Unit Test).

@doganoo doganoo closed this Feb 27, 2020
@doganoo doganoo mentioned this pull request Feb 27, 2020
@duzun
Copy link
Author

duzun commented Feb 28, 2020

You are welcome!

I am glad to contribute when I have more free time.

Until then I don't think it is the right thing to do to close this PR and leave the TimSort implementation broken without any notice.

@doganoo
Copy link
Owner

doganoo commented Nov 1, 2020

Hey there,

it has been a while :)

I think it is not a good idea to prove broken code since it could let people assume that this behaviour is wanted.

Instead, I added a var_dump to the class' constructor. It is not a good solution, I know. But it clearly says that it is broken instead of allowing people to assume.

PR's are highly welcome. Please refer to #23 when creating a PR. Thank you in advance :)

@duzun duzun changed the title unit-test to showcase a TimSort issue unit-test to catch a TimSort issue Nov 1, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants