Skip to content

Promote DateTime property type to NeoWiki core#805

Merged
malberts merged 2 commits intomasterfrom
promote-datetime-to-core
May 2, 2026
Merged

Promote DateTime property type to NeoWiki core#805
malberts merged 2 commits intomasterfrom
promote-datetime-to-core

Conversation

@malberts
Copy link
Copy Markdown
Collaborator

@malberts malberts commented May 1, 2026

Result of back-and-forth with @malberts.
Context: the NeoWiki codebase, the existing built-in property types
(Text/Url/Number/Select/Relation), and the in-tree RedHerb test extension
where DateTime previously lived.
Written by Claude Code, Opus 4.7 (1M context)

Move the dateTime PropertyType backend from the RedHerb test extension into
NeoWiki core, alongside the other built-in types. The frontend already lived
in NeoWiki, so this completes the migration.

  • DateTimeType and DateTimeProperty move to src/Domain/PropertyType/Types/
    and src/Domain/Schema/Property/.
  • Registered via PropertyTypeRegistry::withCoreTypes() and
    Neo4jValueBuilderRegistry::withCoreBuilders(), using the shared $toScalars
    builder — matching what RedHerb did.
  • RedHerb keeps Color as the canonical extension-point example.
  • The two RedHerb-specific DateTime assertions in NeoWikiRegistrationHookTest
    are dropped; equivalent core coverage is added via the moved
    DateTimePropertyTest, a new DateTimeTypeTest, and an extra
    hasBuilder( 'dateTime' ) assertion in Neo4jValueBuilderRegistryTest.

No behaviour changes.

> Result of back-and-forth with @malberts.
> Context: the NeoWiki codebase, the existing built-in property types
> (Text/Url/Number/Select/Relation), and the in-tree RedHerb test extension
> where DateTime previously lived.
> <sub>Written by Claude Code, `Opus 4.7 (1M context)`</sub>

Move the `dateTime` PropertyType backend from the RedHerb test extension into
NeoWiki core, alongside the other built-in types. The frontend already lived
in NeoWiki, so this completes the migration.

- `DateTimeType` and `DateTimeProperty` move to `src/Domain/PropertyType/Types/`
  and `src/Domain/Schema/Property/`.
- Registered via `PropertyTypeRegistry::withCoreTypes()` and
  `Neo4jValueBuilderRegistry::withCoreBuilders()`, using the shared `$toScalars`
  builder — matching what RedHerb did.
- RedHerb keeps Color as the canonical extension-point example.
- The two RedHerb-specific DateTime assertions in `NeoWikiRegistrationHookTest`
  are dropped; equivalent core coverage is added via the moved
  `DateTimePropertyTest`, a new `DateTimeTypeTest`, and an extra
  `hasBuilder( 'dateTime' )` assertion in `Neo4jValueBuilderRegistryTest`.

No behaviour changes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@malberts malberts marked this pull request as ready for review May 1, 2026 20:46
@alistair3149
Copy link
Copy Markdown
Member

Result of code review with @alistair3149.
Context: the NeoWiki codebase, specifically the sibling property type registrations and test patterns under src/Domain/PropertyType/Types/ and tests/phpunit/Domain/....
Written by Claude Code, Opus 4.7 (1M context)

Reviewed the diff against the surrounding code. No blockers — the move is mechanically correct, behaviour-preserving ($toScalars matches the prior fn ( $value ) => $value->toScalars()), namespaces line up with sibling types, the frontend typeName = 'dateTime' matches DateTimeType::NAME, and no DateTime references are left behind in tests/RedHerb/.

A couple of minor test-shape items worth tracking, both introduced by this PR:

  1. Duplicate testDateTimeTypeTest::testBuildPropertyDefinitionFromJsonReturnsDateTimeProperty is functionally identical to DateTimePropertyTest::testBuildPropertyDefinitionFromJsonViaType (same setup, same 'minimum' value, same assertions). Sibling *TypeTest classes (NumberTypeTest, UrlTypeTest, RelationTypeTest, TextTypeTest) are deliberately tiny and only cover getDisplayAttributeNames(). Suggest dropping the duplicate from DateTimeTypeTest to match.
  2. testValueTypeIsString in DateTimeTypeTest is uneven with siblings — none of the other *TypeTests assert getValueType(). Either drop it for consistency, or extend the same assertion to siblings in a follow-up.

Optional follow-up (not specific to this PR): DateTimePropertyTest could adopt the assertSerializationDoesNotChange round-trip pattern used by NumberPropertyTest / TextPropertyTest via PropertyTestCase, to exercise the registry-driven PropertyDefinition::fromJson() path.

The prior version had four methods, but the equivalent assertions either
duplicated DateTimePropertyTest (buildPropertyDefinitionFromJson coverage,
property type name) or were inconsistent with sibling type tests
(NumberTypeTest, UrlTypeTest, etc.) which only assert getDisplayAttributeNames().

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@malberts malberts merged commit 046c339 into master May 2, 2026
6 checks passed
@malberts malberts deleted the promote-datetime-to-core branch May 2, 2026 10:03
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