adityamparikh commented on PR #132:
URL: https://github.com/apache/solr-mcp/pull/132#issuecomment-5334021275
Closing this as superseded by #106 and #113, both of which have now landed
on `main`.
This PR originated from a review sub-thread on #131 asking why the DTO
records used
`java.util.Date` instead of `java.time.Instant`. My answer here was "the
migration isn't
needed, because every `Date` field is cargo-culted noise that should just be
deleted."
The project went the other way, and I think that's the better call:
- **#113** (`refactor: replace mutable Date with Instant in DTO records`)
performed the
migration directly, keeping the timestamps and modernizing their type.
That resolves the
original review question.
- **#106** (`refactor: remove dead fields and unused FieldStats type`)
already removed
`SolrHealthStatus.solrVersion` and `SolrHealthStatus.status` — two of the
seven fields
this PR targeted.
What remained unique to this PR was removing
`CollectionCreationResult.success` / `.message`,
plus deleting `SolrMetrics.timestamp`, `SolrHealthStatus.lastChecked` and
`CollectionCreationResult.createdAt` — the three fields #113 had just
deliberately converted
to `Instant`. Rebasing would mean resolving those conflicts by reverting a
considered decision
that was merged on purpose, which isn't a merge conflict so much as a
disagreement, and not
one worth relitigating for the wire-format savings involved.
If the `CollectionCreationResult.success` / `.message` removal is still
wanted on its own
merits (both are constant — `success` is always `true` because failures
throw before the
result is built, and `message` is always the literal `"Collection created
successfully"`),
that's a much smaller, self-contained change and I'm happy to open a fresh
PR scoped to just
that. Leaving it out for now rather than assuming.
One incidental finding worth recording: after rebasing this branch onto
current `main`, the
build went red in `McpClientIntegrationTestBase.createShowsCollection()`.
#131 added that test
after this branch was cut, and it reintroduced the same
`text.contains("success") || text.contains("true")` assertion pattern that
this PR invalidates.
The rebase was textually clean because the two changes touch different lines
of the same file.
That assertion is fine on `main` today, since
`CollectionCreationResult.success` still exists —
but it's coupled to a field's presence rather than to the tool actually
succeeding, so it would
break again under any future change that drops the field. Asserting on the
echoed collection
name is the more durable form.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
