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

feat: replace CircularArray with ExposedQueue #51

Merged
merged 16 commits into from
May 27, 2024

Conversation

JoenvandeVorle
Copy link
Contributor

@JoenvandeVorle JoenvandeVorle commented May 8, 2024

Description

This changes the MemoryBelief to use an exposed queue instead of a circular array.

Testability

Run the unit tests in ExposedQueueTests and MemoryBeliefTests.

Checklist

  • Set the proper pull request name
  • Merged main into your branch

@JensSteenmetz JensSteenmetz changed the title refactor: circular array to exposed queue refactor: replace CircularArray with ExposedQueue May 9, 2024
@JoenvandeVorle JoenvandeVorle changed the base branch from main to dev May 13, 2024 12:12
@JoenvandeVorle JoenvandeVorle self-assigned this May 13, 2024
Copy link
Contributor

@joachimdekker joachimdekker left a comment

Choose a reason for hiding this comment

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

Left a few comments.

Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
@JensSteenmetz JensSteenmetz changed the title refactor: replace CircularArray with ExposedQueue feat: replace CircularArray with ExposedQueue May 15, 2024
Copy link
Member

@JensSteenmetz JensSteenmetz left a comment

Choose a reason for hiding this comment

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

Overall good work! Left a few comments.

Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
Aplib.Tests/Belief/ExposedQueueTests.cs Outdated Show resolved Hide resolved
Aplib.Tests/Belief/ExposedQueueTests.cs Outdated Show resolved Hide resolved
JoenvandeVorle and others added 4 commits May 15, 2024 16:49
move private method, remove debug log, add exception check.
work in progress commit so I can work on it on my PC
added exceptions and comments, moved some code, removed remove
Copy link
Member

@JensSteenmetz JensSteenmetz left a comment

Choose a reason for hiding this comment

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

A few more comments

Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@joachimdekker joachimdekker left a comment

Choose a reason for hiding this comment

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

Looks solid, but I left some architectural comments and found a couple of test smells.

Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
Aplib.Tests/Belief/ExposedQueueTests.cs Outdated Show resolved Hide resolved
Aplib.Tests/Belief/ExposedQueueTests.cs Outdated Show resolved Hide resolved
Aplib.Tests/Belief/ExposedQueueTests.cs Outdated Show resolved Hide resolved
Aplib.Tests/Belief/MemoryBeliefTests.cs Outdated Show resolved Hide resolved
Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
also added/improved tests and resolved some PR feedback
Copy link
Member

@JensSteenmetz JensSteenmetz left a comment

Choose a reason for hiding this comment

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

Left one more remark

Aplib.Core/Belief/MemoryBelief.cs Outdated Show resolved Hide resolved
Aplib.Core/ExposedQueue.cs Outdated Show resolved Hide resolved
JensSteenmetz
JensSteenmetz previously approved these changes May 20, 2024
Copy link
Member

@JensSteenmetz JensSteenmetz left a comment

Choose a reason for hiding this comment

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

LGTM!!

joachimdekker
joachimdekker previously approved these changes May 22, 2024
Copy link
Contributor

@joachimdekker joachimdekker left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@JensSteenmetz JensSteenmetz left a comment

Choose a reason for hiding this comment

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

LGTM!!!

Copy link

@JoenvandeVorle JoenvandeVorle merged commit 36fbbb3 into dev May 27, 2024
2 checks passed
@JoenvandeVorle JoenvandeVorle deleted the refactor/37-circularArray-to-exposedQueue branch May 27, 2024 15:57
Copy link

🎉 This PR is included in version 1.11.0-dev.6 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Copy link

github-actions bot commented Jun 5, 2024

🎉 This PR is included in version 1.11.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Copy link

github-actions bot commented Jun 5, 2024

🎉 This PR is included in version 1.11.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants