Skip to content

Add EuclideanDistance.unsafe, module configuration, typeclass instances for Distance, and PixelDefinition and Pixels3D tests from looptrace#29

Merged
cchlanger merged 11 commits into
mainfrom
vr/add-eucl-dist-unsafe
Jul 7, 2026
Merged

Add EuclideanDistance.unsafe, module configuration, typeclass instances for Distance, and PixelDefinition and Pixels3D tests from looptrace#29
cchlanger merged 11 commits into
mainfrom
vr/add-eucl-dist-unsafe

Conversation

@vreuter

@vreuter vreuter commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

TODO:

  • CI passing
  • CHANGELOG update
  • version update (drop SNAPSHOT)

Updated base feature branch from #28

@vreuter vreuter requested review from cchlanger and valesori July 5, 2026 21:39
cchlanger added 3 commits July 7, 2026 13:29
Drop Java 11 and 19 from the CI build matrix, keeping only the LTS
releases Temurin 17 and 21. Exclude the macOS + Temurin 17 combination
so macOS only runs on Java 21, matching the looptrace CI setup.

Java 19 (non-LTS, EOL March 2023) was the source of the
SSLHandshakeException when resolving edu.ucar:cdm-core from Unidata's
artifact server: its bundled cacerts predates the Sectigo Root R46
trust anchor. Java 17 and 21 already trust that root, so no truststore
workaround is needed.

Also bump primaryJavaVersion to 21 so the scalafmt/scalafix checks run
on the new primary JVM, and add .DS_Store to .gitignore.
Correct EuclideanDistance.unsafe scaladoc to state it interprets (not
"tries to interpret") a length and throws IllegalArgumentException on a
negative value, matching its actual behavior. Document that the
Show[Distance] format is chosen to roundtrip through Distance.parse via
squants' Length parser. Remove a stray blank line and add a trailing
newline in .gitignore.
@cchlanger

Copy link
Copy Markdown
Member

Code Review — Approving ✅

Reviewed the full diff for #29. Solid, well-tested work — clean typeclass usage, good coverage, and consistent propagation of the error-message change.

What this PR delivers

  • EuclideanDistance.unsafe(Length) — a validating wrapper that throws on a negative length, complementing the existing Either-returning parse.
  • New configuration module with pureconfig/scopt parsing of pixel definitions (PixelDefinition, Pixels3D).
  • Show / SimpleShow instances for geometry.Distance.
  • Tooling: sbt → 1.12.13, plugin bumps, and a CI Java matrix trimmed to LTS releases Temurin 17 & 21.
  • Version bump to 0.6.0 + changelog.

Strengths

  • Good test coverage for the new behavior: unsafe is covered for the type restriction (assertTypeError/assertCompiles), the happy path, and the failure path with an exact error-message assertion.
  • Error handling in the pureconfig readers is clean — emap + leftMap into CannotConvert with value/toType/because is exactly the right pattern.
  • The error-message change is consistently propagated (source + TestDistanceJsonCodec + TestDistance), so nothing silently drifts.

Should-fix / worth confirming

  1. EuclideanDistance.unsafe scaladoc was misleading — "Try to interpret" read like it returned an Either/Option, but it throws IllegalArgumentException on a negative length. ✅ Fixed: reworded and added a @throws tag.
  2. Show[Distance] roundtrip depends on squants' parser — the roundtrip test uses Double.MaxValue, so large values render in scientific notation; the guarantee rests on squants' parsing grammar. ✅ Documented this dependency in a comment.

Nits

  1. Test-helper duplicationmaxSize, genMicroOrNano, MicroOrNano, genPlusMinusMillion are duplicated between the imaging and configuration suites. Different modules, so sharing would need the testing module — probably not worth it now, just noting the drift risk.
  2. minSuccessful(10000) in two property tests is heavier than needed for verifying linear scaling; 1000 would likely give the same confidence at lower CI cost.

Questions

  • deriveReader[Pixels3D] over private x/y/z fields — derivation works via the product Mirror regardless of field visibility, and the test confirms it. Just confirming it's intentional that x/y/z become part of the public config API despite the fields being private.

Follow-up fixes applied after review

  • Corrected the unsafe scaladoc (item 1) and added a @throws tag.
  • Documented the Show[Distance] roundtrip dependency (item 2).

Verification

  • Full aggregated test runs green locally (68 tests / 20 suites), including the roi and testing suites that were previously skipped when zarr dependency resolution failed.
  • Confirmed the earlier CI failure's root cause was Java 19's outdated truststore (missing the Sectigo Root R46 anchor); dropping 19 resolves it, and 17/21 already trust the root, so no truststore workaround is needed.

Good to merge.

@cchlanger cchlanger merged commit 5789213 into main Jul 7, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants