Conversation
…objects with defined spawning data - Added GetPool<T>() && TryGetPool<T>() methods to PoolService to allow requesting the pool object maintained by the pool service. **Changed**: - Removed IsSpawned<T>() method from PoolService because is not a fundamental function and can now be accessed from the Pool requested from GetPool() - Now Spawn<T>(T data) also invokes OnSpawn() without data so objects that implement IPoolEntitySpawn have the entire behaviour lifecycle
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe changes in this pull request introduce a new version (0.11.0) of the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
WalkthroughThe changes include the addition of new methods to the Changes
Uplevel your code reviews with CodeRabbit ProCodeRabbit ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, CodeRabbit Pro is free for open source projects. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (4)
- CHANGELOG.md (1)
- Runtime/ObjectPool.cs (13)
- Runtime/PoolService.cs (2)
- Tests/Editor/EditMode/PoolServiceTest.cs (2)
Files ignored due to filter (1)
- package.json
Files not summarized due to errors (1)
- Runtime/ObjectPool.cs (diff tokens exceeds limit)
Review comments generated (18)
- Review: 6
- LGTM: 12
Tips
Chat with 
CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| /// <typeparam name="T">The type of objects in the pool.</typeparam> | ||
| /// <returns>The pool of objects.</returns> | ||
| /// <exception cref="ArgumentException">Thrown if the pool does not exist.</exception> | ||
| IObjectPool<T> GetPool<T>() where T : class; |
There was a problem hiding this comment.
The GetPool<T>() method in the new code is missing error handling for the case when the pool does not exist. It should throw an ArgumentException in that case, similar to the old code.
- return pool as IObjectPool<T>;
+ if (pool == null)
+ {
+ throw new ArgumentException("The pool was not initialized for the type " + typeof(T));
+ }
+ return (IObjectPool<T>)pool;|
|
||
| private IObjectPool<T> GetPool<T>() where T : class | ||
| { | ||
| if (!_pools.TryGetValue(typeof(T), out var pool)) | ||
| { | ||
| throw new ArgumentException("The pool was not initialized for the type " + typeof(T)); | ||
| } | ||
|
|
||
| return pool as IObjectPool<T>; | ||
| _pools.Clear(); | ||
| } |
There was a problem hiding this comment.
The Clear() method in the new code is missing a newline at the end of the file. Please add a newline character (\n) at the end of line 155.
{
pool.Value.Dispose();
}
_pools.Clear();
}Overall, the changes made to the Runtime/PoolService.cs file seem reasonable. The addition of the Spawn<T>(T data) method and the GetPool<T>() and TryGetPool<T>() methods provide more flexibility and convenience. The removal of the IsSpawned<T>() method makes sense since it can now be accessed from the pool object itself. The modifications to the existing methods appear to be straightforward and do not introduce any issues.
| [Test] | ||
| public void TryGetPool_Successfully() | ||
| { | ||
| Assert.True(_poolService.TryGetPool<IMockPoolableEntity>(out var pool)); | ||
| Assert.AreEqual(_pool, pool); | ||
| } |
There was a problem hiding this comment.
The addition of the TryGetPool_Successfully test method and the modification of the TryGetPool assertion in line 31 are appropriate. However, it would be beneficial to add a negative test case where TryGetPool fails to retrieve a pool for an invalid type.
[Test]
public void TryGetPool_Unsuccessfully()
{
Assert.False(_poolService.TryGetPool<InvalidType>(out var pool));
}| [Test] | ||
| public void GetPool_Successfully() | ||
| { | ||
| Assert.AreEqual(_pool, _poolService.GetPool<IMockPoolableEntity>()); | ||
| } |
There was a problem hiding this comment.
The addition of the GetPool_Successfully test method and the modification of the GetPool assertion in line 38 are appropriate.
| [Test] | ||
| public void AddPool_Successfully() | ||
| { | ||
| Assert.True(_poolService.HasPool<IMockPoolableEntity>()); | ||
| Assert.True(_poolService.TryGetPool<IMockPoolableEntity>(out _)); |
There was a problem hiding this comment.
The addition of the AddPool_Successfully test method is appropriate. However, the assertion in line 44 can be improved. Instead of using TryGetPool with an unused out variable, you can directly use HasPool to check if the pool exists.
[Test]
public void AddPool_Successfully()
{
Assert.True(_poolService.HasPool<IMockPoolableEntity>());
}|
|
||
| Assert.Throws<ArgumentException>(() => _poolService.GetPool<IMockPoolableEntity>()); |
There was a problem hiding this comment.
The modification of the RemovePool_Successfully test method is appropriate. However, the assertion in line 106 can be improved. Instead of using GetPool and expecting an exception, you can directly use HasPool to check if the pool has been removed.
[Test]
public void RemovePool_Successfully()
{
_poolService.RemovePool<IMockPoolableEntity>();
Assert.False(_poolService.HasPool<IMockPoolableEntity>());
}Overall, the changes made to Tests/Editor/EditMode/PoolServiceTest.cs are reasonable, but there are opportunities to improve the assertions for better clarity and simplicity.
1 participant
Changed:
Summary by CodeRabbit
PoolServicefor spawning objects with defined data and accessing pool objects.IsSpawned<T>()method fromPoolService.Spawn<T>(T data)to invokeOnSpawn()without data.GetPool<T>(),TryGetPool<T>(), andRemovePool<T>()inPoolService.PoolService.Summary by CodeRabbit
Release Notes for Version 0.11.0
New Features
Spawn<T>(T data),GetPool<T>(), andTryGetPool<T>()for enhanced pool management.Bug Fixes
Tests
PoolService.Chores