Remove unused methods and improve functionality in core classes#223
Conversation
Add DynamicOps parameter and implement encode-transform-decode logic so the rewrite function is actually applied. Document the breaking signature change in the v0.5-to-v1.0 migration guide.
…, #119) Parent types were fetched but never copied into the child registry, requiring subclasses to re-register all types. Now iterates parent types and registers them before registerTypes() runs. Add double-checked locking with volatile for thread-safe lazy initialization of the type registry. Use a separate buildingTypes field during construction to avoid publishing an incomplete registry.
…ss for improved object comparison, hashing, and debugging. (#120)
…th errors gracefully (#121)
Splatcrafter
added
bug
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
…haosException` Standardize exception handling by replacing direct `ChaosException` references with `FixException`, which encapsulates `ChaosException`. Update assertions and comments accordingly.
…reverse()` composition in `Iso`
…l checks and streamlining `asString` handling
There was a problem hiding this comment.
Pull request overview
This PR targets multiple correctness and API issues across the datafixer API/core by fixing previously broken behaviors (no-op fixes, thread-unsafe schema init, invalid finder indices), improving debugging ergonomics, and documenting a breaking API change in the migration guide.
Changes:
- Fix/extend optics and traversal behavior (
Iso.reverse().compose(),Finder.index(-1)validation,Typed.getAt()missing-path handling). - Make
Schema.types()lazy initialization thread-safe and implement parent type inheritance. - Make
Fixes.fixTypeEverywhere()actually apply dynamic rewrites and update migration docs/tests accordingly.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/migration/v0.5-to-v1.0.md | Documents breaking signature change for Fixes.fixTypeEverywhere() and adds migration checklist entry |
| aether-datafixers-functional-tests/.../RandomFailuresChaosIT.java | Updates expectations to FixException wrapping for chaotic failure scenarios |
| aether-datafixers-api/src/test/.../IsoTest.java | Adds regression test ensuring reverse() supports compose() |
| aether-datafixers-api/src/test/.../FinderTest.java | Adds regression test rejecting negative indices |
| aether-datafixers-api/src/main/.../Typed.java | Fixes missing-path handling in getAt() by returning a DataResult.error |
| aether-datafixers-api/src/main/.../Schema.java | Adds DCL/volatile for thread-safe lazy init and implements parent type inheritance |
| aether-datafixers-api/src/main/.../Iso.java | Removes reverse() implementation that previously blocked composition |
| aether-datafixers-api/src/main/.../Finder.java | Adds upfront argument validation for negative index |
| aether-datafixers-api/src/main/.../Fixes.java | Implements actual transformation logic for fixTypeEverywhere() with DynamicOps |
| aether-datafixers-api/src/main/.../TaggedDynamic.java | Adds equals/hashCode/toString for better comparison and diagnostics |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return encodeResult.flatMap(dynamic -> { | ||
| final Dynamic<?> transformed = rewrite.apply(dynamic); | ||
| return ((Type) type).read(transformed); | ||
| }).map(newValue -> new Typed<>((Type) type, newValue)).result(); |
There was a problem hiding this comment.
fixTypeEverywhere currently turns encode/decode failures into Optional.empty() via DataResult.result(). That makes the rule silently behave like a non-match (no-op) on actual transformation errors. Consider propagating failures by throwing an exception (e.g., getOrThrow(...) with a message including the rule name/type) instead of swallowing them.
| return encodeResult.flatMap(dynamic -> { | |
| final Dynamic<?> transformed = rewrite.apply(dynamic); | |
| return ((Type) type).read(transformed); | |
| }).map(newValue -> new Typed<>((Type) type, newValue)).result(); | |
| final DataResult<Typed<?>> result = encodeResult.flatMap(dynamic -> { | |
| final Dynamic<?> transformed = rewrite.apply(dynamic); | |
| return ((Type) type).read(transformed); | |
| }).map(newValue -> new Typed<>((Type) type, newValue)); | |
| final Typed<?> value = result.getOrThrow(false, error -> { | |
| throw new IllegalStateException( | |
| "Error applying fixTypeEverywhere rule '" + name + "' to type " + type + ": " + error | |
| ); | |
| }); | |
| return Optional.of(value); |
| private TypeRegistry buildTypes() { | ||
| final TypeRegistry registry = this.createTypeRegistry(); | ||
| this.types = registry; | ||
| this.buildingTypes = registry; | ||
|
|
||
| // Inherit types from parent if present | ||
| if (this.parent != null) { | ||
| // Copy types from parent | ||
| this.parent.types(); | ||
| // Parent types are already registered in parent's registry | ||
| // For now, we don't copy - subclass must re-register all types it needs | ||
| final TypeRegistry parentTypes = this.parent.types(); | ||
| for (final TypeReference ref : parentTypes.references()) { | ||
| final Type<?> parentType = parentTypes.get(ref); | ||
| if (parentType != null) { | ||
| registry.register(parentType); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Let subclass register types | ||
| this.registerTypes(); | ||
| this.buildingTypes = null; | ||
|
|
There was a problem hiding this comment.
buildTypes() sets buildingTypes but only clears it on the happy path. If inheriting parent types or registerTypes() throws, buildingTypes will remain non-null and the Schema instance is left in an inconsistent state (later registerType(...) calls may unexpectedly succeed). Wrap the body in try/finally to ensure buildingTypes is always cleared.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| */ | ||
| @NotNull | ||
| static Finder<Object> index(final int index) { | ||
| Preconditions.checkArgument(index >= 0, "index must not be negative, got: %s", index); | ||
| return new Finder<>() { |
There was a problem hiding this comment.
The Javadoc for Finder.index(...) says negative indices cause get() to return null, but the method now throws IllegalArgumentException at construction time for index < 0. Please update the Javadoc to reflect the new contract (and clarify that only out-of-range positive indices return null/leave root unchanged).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| return encode(ops).flatMap(dynamic -> { | ||
| final Dynamic<?> found = finder.get(dynamic); | ||
| if (found == null) { | ||
| return null; | ||
| return DataResult.error("Path not found: " + finder); | ||
| } | ||
| return found.convert(ops); | ||
| return DataResult.success(found.convert(ops)); |
There was a problem hiding this comment.
getAt(...) now returns DataResult.error("Path not found: ...") when the finder doesn't locate a path. The method Javadoc still states the result may contain null when not found; please update the @return documentation to match the new error-based behavior.
There was a problem hiding this comment.
@copilot commit to this branch to apply changes based on this feedback
|
@Splatcrafter I've opened a new pull request, #224, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Splatcrafter <83497438+Splatcrafter@users.noreply.github.com> Agent-Logs-Url: https://github.com/aether-framework/aether-datafixers/sessions/f3b66d1c-d3d8-496d-a7b9-a4625bf96b66
Fix stale `@return` Javadoc on `Typed.getAt` to reflect error-based behavior
|
@Splatcrafter I've opened a new pull request, #225, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@Splatcrafter I've opened a new pull request, #226, to work on those changes. Once the pull request is ready, I'll request review from you. |
…ixers/api/fix/Fixes.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…her-one [WIP] [WIP] Address feedback on removing unused methods
…, not null from get() Co-authored-by: Splatcrafter <83497438+Splatcrafter@users.noreply.github.com> Agent-Logs-Url: https://github.com/aether-framework/aether-datafixers/sessions/2ba3dec6-7d25-47ab-b4b3-624a4c124efc
…pr-223-another-one Revert "[WIP] [WIP] Address feedback on removing unused methods"
Fix Finder.index() Javadoc to reflect IllegalArgumentException contract for negative indices
…ataResult#result` and removing redundant error handling.
|
Note for me: NEVER use Copilot for PR's ever again!!!💀😭 |
Summary
This PR removes unused methods, fixes bugs, improves functionality in several classes, and resolves various underlying issues. Key updates include enhancing thread safety, improving error handling, and refining object comparison methods.
Type of Change
Related Issues
Closes #117
Closes #118
Closes #119
Closes #120
Closes #121
Closes #122
Closes #123
Changes
composemethod from reversedIsoimplementation.Finder.indexmethod.Typed.encodeAndGetto returnDataResultand handle missing path errors.equals,hashCode, andtoStringinTaggedDynamicfor better debugging and object comparison.Schema.buildTypes()for proper parent type inheritance and added thread safety with double-checked locking.Fixes.fixTypeEverywhere()to correctly apply transformations.Verification
Breaking Changes
Fixes.fixTypeEverywhere()documented in the v0.5-to-v1.0 migration guide.Schema.buildTypes()involve new behavior for parent type inheritance and lazy initialization logic.Checklist