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

Priority Queue Multithreaded Test #15

Merged
merged 3 commits into from
Dec 27, 2023
Merged

Priority Queue Multithreaded Test #15

merged 3 commits into from
Dec 27, 2023

Conversation

DrDub
Copy link
Contributor

@DrDub DrDub commented Dec 23, 2023

I looked at your other test cases, hopefully this one works well out of the box.

@DNedic
Copy link
Owner

DNedic commented Dec 24, 2023

Thanks for the contribution @DrDub , unfortunately the code is not very readable and clear in my opinion. Perhaps you could refactor it or comment on the intended behavior.

Of course, it doesn't have to match code readability of the library code as it's a test, but I feel uncomfortable adding this test as it would be hard to debug quickly.

@DrDub
Copy link
Contributor Author

DrDub commented Dec 25, 2023

Thanks for taking the time to explain.

Your other test cases are short and self-documenting. I couldn't come up with a multi-threaded test on par with that so I guess this test case will have to be explicitly documented. I have added a few paragraphs of text and will be happy to add more or we can discuss the merits of the approach.

One of the nice things of documenting it is that I realized I could spare one of the loops so it is tighter now 👍

@DNedic
Copy link
Owner

DNedic commented Dec 27, 2023

Thank you for your contribution @DrDub! I will be also pushing a commit with some cleanups so you can watch out for that if you're interested after the merge.

@DNedic DNedic merged commit d2d8225 into DNedic:main Dec 27, 2023
1 check passed
@DrDub
Copy link
Contributor Author

DrDub commented Dec 27, 2023

Sure, will do! It seems #4 is close to completion by now.

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.

2 participants