major changes and feature additions #1

Merged
lentil merged 11 commits from claude_code_work into main 2025-06-15 14:08:40 -05:00
Collaborator

This PR represents a day's work with Claude Code. See todo.md for a list of features added. Additionally some paradigm shifts were integrated, and the filesystem was reorganized. This is too big to properly review but I figure it'd be helpful to treat it as a PR rather than just pushing to master.

This PR represents a day's work with Claude Code. See todo.md for a list of features added. Additionally some paradigm shifts were integrated, and the filesystem was reorganized. This is too big to properly review but I figure it'd be helpful to treat it as a PR rather than just pushing to master.
myk added 11 commits 2025-06-10 12:14:13 -05:00
lentil requested changes 2025-06-11 13:00:29 -05:00
lentil left a comment
Owner

Nicely done! I find it very rewarding that my code was written and structured well enough to enable these extensions. I commented on a few minor points.

Nicely done! I find it very rewarding that my code was written and structured well enough to enable these extensions. I commented on a few minor points.
@ -0,0 +1,247 @@
import * as _RhizomeImports from "../src";
Owner

This line appears to do nothing

This line appears to do nothing
lentil marked this conversation as resolved
@ -0,0 +58,4 @@
const result = resolver.resolve();
expect(result).toBeDefined();
// Should resolve deterministically (likely based on delta processing order)
Owner

Comment is outdated, this seems to use a more robust tie breaking algorithm, not relying on processing order

Comment is outdated, this seems to use a more robust tie breaking algorithm, not relying on processing order
lentil marked this conversation as resolved
@ -0,0 +1,492 @@
import * as _RhizomeImports from "../src";
Owner

This line appears to do nothing

This line appears to do nothing
lentil marked this conversation as resolved
@ -0,0 +443,4 @@
expect(stats.negationDeltas).toBe(0); // No negations for this entity
});
it('should handle multiple negations and un-negations', () => {
Owner

and un-negations

I'm not seeing anything implementing or testing the negation of a negation

> and un-negations I'm not seeing anything implementing or testing the negation of a negation
lentil marked this conversation as resolved
@ -0,0 +251,4 @@
while (currentView.nestedObjects.next && currentView.nestedObjects.next.length > 0) {
currentView = currentView.nestedObjects.next[0];
depth++;
if (depth >= 5) break; // Prevent infinite loop
Owner

This condition means the subsequent test of depth will always pass, regardless of actual depth

This condition means the subsequent test of depth will always pass, regardless of actual depth
lentil marked this conversation as resolved
@ -4,0 +337,4 @@
'invalid-operator': [{ 'var': 'age' }, 25]
});
// Should not crash, may return empty results or skip problematic entities
Owner

Seems like we should probably return an error to the caller in this case?

Seems like we should probably return an error to the caller in this case?
lentil marked this conversation as resolved
@ -0,0 +1,501 @@
import * as _RhizomeImports from "../src";
Owner

This line appears to do nothing

This line appears to do nothing
lentil marked this conversation as resolved
@ -0,0 +1 @@
MANIFEST-000030
Owner

I think data/ should probably be excluded from the repo by .gitignore

I think `data/` should probably be excluded from the repo by `.gitignore`
lentil marked this conversation as resolved
@ -0,0 +124,4 @@
const entity = this.lossless.domainEntities.get(entityId);
if (!entity) continue;
// Check if entity has deltas for any required property
Owner

Maybe this should only include items with all required properties?

Maybe this should only include items with _all_ required properties?
lentil marked this conversation as resolved
@ -0,0 +249,4 @@
}
/**
* Extract reference value (target ID) from a delta for a given property
Owner

for a given property

It seems like a bug that propertyId is not referenced in this function

> for a given property It seems like a bug that `propertyId` is not referenced in this function
lentil marked this conversation as resolved
@ -0,0 +199,4 @@
}
// Common schema patterns
export const CommonSchemas = {
Owner

It looks like CommonSchemas is only used in __tests__ and should probably be moved there

It looks like `CommonSchemas` is only used in `__tests__` and should probably be moved there
lentil marked this conversation as resolved
@ -0,0 +270,4 @@
}
resolve(state: { min?: number }): PropertyTypes {
return state.min || 0;
Owner

I wonder if this is safe -- maybe if no values are found the result should be omitted

I wonder if this is safe -- maybe if no values are found the result should be omitted
lentil marked this conversation as resolved
@ -0,0 +106,4 @@
return res;
};
// Override resolve to build accumulator on-demand if needed
Owner

Maybe this belongs in the Lossy class itself?

Maybe this belongs in the `Lossy` class itself?
lentil marked this conversation as resolved
Owner

I resolved all my own feedback from this PR on branch pr-1-feedback. I'll merge this PR and open a new one.

I resolved all my own feedback from this PR on branch pr-1-feedback. I'll merge this PR and open a new one.
lentil merged commit 04212e87e6 into main 2025-06-15 14:08:40 -05:00
Sign in to join this conversation.
No Reviewers
No Label
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: lentil/rhizome-node#1
No description provided.