-
-
Notifications
You must be signed in to change notification settings - Fork 0
v0.1.0 #3
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
v0.1.0 #3
Conversation
…lity connected to adapter, and set Symbol.iterator to method.
…ssign only adapter in base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR represents version 0.1.0 of the collection library, implementing a significant architectural refactoring where default functionality is moved to the core abstract class, the base class focuses on adapter assignment, and the iterator is changed from a getter to a method.
Key changes:
- Refactored
CollectionCorefrom abstract methods to concrete implementations that delegate to an adapter - Changed
Symbol.iteratorfrom a getter property to a method across all relevant classes - Updated method signatures to accept rest parameters (e.g.,
add(...element: E[]),delete(...element: E[]),has(...element: E[]))
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/core/lib/collection.core.ts | Moved from abstract method declarations to concrete implementations that delegate to adapter; iterator changed to method |
| src/core/lib/collection.base.ts | Simplified to focus on adapter instantiation and storage; removed duplicate method implementations now in core |
| src/lib/collection.class.ts | Updated template parameter constraints from ...args: any[] to ...elements: E[] for better type safety |
| src/adapter/lib/set.adapter.ts | Changed iterator from getter to method; updated method signatures to accept rest parameters; improved type constraints |
| src/test/collection.spec.ts | Added iterator usage demonstration with for-of loop |
| package.json | Bumped version to 0.1.0 and updated peer dependency to @typedly/collection ^2.0.0 |
| package-lock.json | Updated lockfile to reflect new version and dependencies |
Comments suppressed due to low confidence (1)
src/core/lib/collection.base.ts:24
- The JSDoc comment states
@protectedbut the getter is declared aspublicon line 25. Either change the access modifier toprotectedor update the documentation to reflect@public.
/**
* @description The protected getter and setter for the adapter.
* @protected
* @type {A}
*/
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @template E type in collection. | ||
| * @template T of the collection. |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Template parameter descriptions are incomplete. Consider making them more consistent with other files (e.g., @template E The type of elements in collection. and @template T The type of the collection.)
| for (const element of collection) { | ||
| console.log(`element: `, element); | ||
| } | ||
|
|
||
| console.log(`size: `, collection.size); // Output: 5 | ||
|
|
||
| describe("Collection SetAdapter", () => { |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Console.log statements in test files can pollute test output. Consider either removing these statements or moving them into a proper test case if you want to verify the iterator functionality.
| for (const element of collection) { | |
| console.log(`element: `, element); | |
| } | |
| console.log(`size: `, collection.size); // Output: 5 | |
| describe("Collection SetAdapter", () => { | |
| // Iterator functionality test | |
| describe("Collection SetAdapter", () => { | |
| test("iterator yields expected elements", () => { | |
| const elements = Array.from(collection); | |
| expect(elements.sort()).toEqual([1, 2, 3, 27, 33].sort()); | |
| }); |
| for (const element of collection) { | ||
| console.log(`element: `, element); | ||
| } |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The iterator functionality added in this PR should have proper test coverage. Consider adding a test case that verifies the iterator returns the expected elements, such as: test('iterator yields all elements', () => { const elements = [...collection]; expect(elements).toContain(1); ... });
| return this.adapter[Symbol.iterator](); | ||
| } | ||
|
|
||
| protected abstract get adapter(): A |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing semicolon at the end of the abstract getter declaration. TypeScript requires semicolons for abstract property declarations.
| protected abstract get adapter(): A | |
| protected abstract get adapter(): A; |
Uh oh!
There was an error while loading. Please reload this page.