Skip to content

chore: remove dead code - #147

Closed
JosteinGj wants to merge 15 commits into
mainfrom
chore/remove-dead-code
Closed

chore: remove dead code#147
JosteinGj wants to merge 15 commits into
mainfrom
chore/remove-dead-code

Conversation

@JosteinGj

@JosteinGj JosteinGj commented Sep 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

No public signature changes. One behaviour change, in the bindings: a redundant clone on the datapoint-insert path is gone.

Removed

  • every unused import in both crates, and the unused variables. Two of those were hiding something:
    • ts2 in test_datapoints was built and never added — its add_item sat commented out directly beneath it
    • per_series was computed and then superseded two lines later by per
  • DataHubEntity::ext_id — no caller. It becomes a true marker trait, so the seven impls lose a method whose value for RelForm was already documented as meaningless ("edges are identified by their endpoints, not by an external id of their own… the From impls only need some borrow"). AGENTS.md updated to match.
  • RetrieveFilter::add_aggregate and set_id; an unused delete_events helper in events::tests; a stale #[allow(dead_code)] that no longer hides anything (verified by deleting it and checking the warning stays away)
  • two commented-out blocks, a stray //todo!(), and a /// doc comment stranded inside a function body — which is why rustc called it an unused doc comment
  • AuthState was private while a pub(crate) field exposed it; it is pub(crate) now rather than silenced with an allow

On that RetrieveFilter pair: aggregates and id are both real api features — the backend's DatapointChildRequest carries them — but every field on RetrieveFilter is pub and set_aggregates remains, so nothing became unreachable. set_external_id staying while set_id goes is an asymmetry worth a second opinion.

Corrected in review — DatapointEpoch is restored (c4507ec)

The first pass removed it. That was wrong, and the reasoning is worth stating so it is not repeated. Its only constructor is private and its fields are pub(crate), which reads as "nothing outside this crate can build one or read one." But the type is pub and src/lib.rs has pub mod generic, so a downstream caller can name it in a type position — DatapointsCollection<DatapointEpoch> — and round-trip it through the derived Serialize/Deserialize without ever touching a field.

So it is reachable public API, the removal was a breaking change on a published crate, and it is excluded by this PR's own rule below. Restored verbatim, private from and dead-code warning included. Making the fields pub so the type is genuinely usable is a separate question.

Also fixed here — the discarded bindings (d42e329)

The first pass _-prefixed seven local bindings rather than deleting them. That is the same move as an #[allow(dead_code)]: it silences the warning and leaves the code unread. Reading them turned up a real cost — insert_datapoints did this in both the sync and the async service:

let _val = result.get_items().clone();
Ok(result.get_items().clone())

a full deep copy of every returned datapoint collection, dropped on the spot, then copied again on the next line, on the ingest path. The other five are let _result = <call>…?; followed by Ok(()), where the ? already propagates and the binding never did anything.

The four PyO3 #[classmethod] class parameters are left exactly as they are: the framework requires the argument, so it cannot be deleted, and renaming it fixes nothing.

Deliberately left alone

The ~83 pub items with no consumer anywhere in this repo, the bindings, or python_tests. This crate is published (cargo publish runs in release.yml) and every module is pub mod, so "no consumer here" is not "no consumer." Those belong on a deprecation list for a minor release, not in this PR. Two of them are advertised in AGENTS.md (ProblemDetail::docs()), which is its own decision.

DatapointEpoch is the case that proves the rule — it was treated as an exception because of its private constructor and pub(crate) fields, and it was not one.

Also left: the pyo3 FromPyObject deprecations and StringOrList visibility warnings in the bindings — pre-existing and a separate refactor.

Stack, 3 of 4. #146 has merged, so this now targets main; #148 and #149 are rebased onto this branch's current head.

No docs impact: no public signature changes.

🤖 Generated with Claude Code

@JosteinGj JosteinGj mentioned this pull request Sep 23, 2026
Merged
JosteinGj and others added 11 commits September 24, 2026 11:25
@JosteinGj @claude
chore: remove dead code
c932a7f 
Nothing here changes behaviour or any public signature; it is what the
compiler and a `pub(crate)` sweep could show was unreachable.

- every unused import and unused variable in both crates. Two of those
  variables were hiding something: `ts2` in `test_datapoints` was built and
  never added (its `add_item` sat commented out beneath it), and
  `per_series` was superseded two lines later by `per`.
- `DataHubEntity::ext_id`, which no caller had. It becomes a true marker
  trait, so the seven impls lose a method whose value for `RelForm` was
  already documented as meaningless ("edges are identified by their
  endpoints"). AGENTS.md updated to match.
- `DatapointEpoch`: `pub`, but its only constructor was private and dead and
  its fields are `pub(crate)`, so nothing outside this crate could build one
  or read it.
- `RetrieveFilter::add_aggregate` and `set_id`, an unused test helper in
  `events::tests`, and a stale `#[allow(dead_code)]` that no longer hides
  anything.
- two commented-out blocks, a stray `//todo!()`, and a `///` doc comment
  stranded inside a function body, which is why rustc called it unused.
- `AuthState` was private while a `pub(crate)` field exposed it; it is
  `pub(crate)` now rather than silenced with an allow.

Deliberately left alone: the roughly eighty `pub` items with no consumer in
this repo. This crate is published, so "no consumer here" is not "no
consumer" — they belong on a deprecation list for a minor release, not in
this commit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: jgjesdal <jostein@intellistream.ai>
@JosteinGj @claude
refactor!: remove the inert PROJECT_NAME and the auth_diagnostics module
c0f70ca 
Two things the SDK carried that no longer do anything.

`auth_diagnostics` existed to explain a 401 the api would not: it decoded
the `organization` claim of the token just sent and reconstructed which of
the validator's branches had rejected it, because the authentication entry
point answered with an empty body. It does not any more — a 401 carries a
problem document whose `detail` names the failed check, in wording that
covers the same five branches. The SDK was appending a near-duplicate of
what the server had already said. The `base64` dependency existed only for
this and goes with it.

The multi-tenant test that asserted on the reconstructed message now
asserts on the server's own wording, so it still pins that the explanation
survives the whole path from entry point to `ResponseError`.

`PROJECT_NAME` was accepted, stored and never read, and the backend has no
such concept on either side of the wire — while AGENTS.md and README.md
both listed it as a supported variable. A config value that reads as real
and affects nothing is worse than an absent one.

BREAKING CHANGE: `DataHubConfig::from_vars` loses its `project_name`
parameter, as do the `DataHubClient` and `AsyncDataHubClient` constructors
in the Python bindings. Callers passing it positionally must drop the
argument; callers passing `project_name=` by keyword must remove it.

Needs a note in datahub-sdk-docs: the env-var list and the client
constructor signatures both mention it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: jgjesdal <jostein@intellistream.ai>
@JosteinGj @claude
fix(python): drop the discarded bindings, one of which cloned twice
cbd5db6 
The dead-code sweep `_`-prefixed seven local bindings instead of deleting
them, which is the same move as an `#[allow(dead_code)]`: it silences the
warning and leaves the code unread. Reading them turns up a real cost.

`insert_datapoints` on the async service did, twice:

    let _val = result.get_items().clone();
    Ok(result.get_items().clone())

a full deep copy of every returned datapoint collection, dropped, then
copied again on the next line — on the ingest path.

The other five are `let _result = <call>…?;` followed by `Ok(())`. The `?`
already propagates, so the binding never did anything.

No behaviour change beyond the removed clone. The four PyO3 `#[classmethod]`
class parameters are left as they are: the framework requires the argument,
so it cannot be deleted, and renaming it fixes nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: jgjesdal <jostein@intellistream.ai>
@JosteinGj @claude
fix: drop the doc comment orphaned by removing auth_diagnostics
b0df19e 
Deleting the `mod auth_diagnostics;` line left its `///` behind, so it
bound to the next item and documented the `blocking` module as "Explaining
an unexplained 401 from the token the SDK already holds." That would have
shipped to docs.rs as the blocking client's description.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: jgjesdal <jostein@intellistream.ai>
@JosteinGj @claude
revert: restore generic::DatapointEpoch
5cf32f7 
The dead-code sweep removed it as unreachable from outside the crate: its
only constructor is private and its fields are `pub(crate)`. But the type
itself is `pub` and `src/lib.rs` has `pub mod generic`, so a downstream
caller can still name it in a type position — `DatapointsCollection<
DatapointEpoch>` — and round-trip it through the derived `Serialize` /
`Deserialize` without ever touching a field.

That makes the removal a breaking change on a published crate, which the
sweep's own commit message says it is not, and which its own rule excludes:
`pub` items with no consumer in this repo belong on a deprecation list for a
minor release, not in a dead-code sweep.

Restored verbatim, including the private `from` and the dead-code warning it
carries. Making the fields `pub` so the type is actually usable is a separate
question.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: jgjesdal <jostein@intellistream.ai>
@JosteinGj @claude
docs: give the crate a landing page and build docs.rs the way the REA…
617fcb8 
…DME reads

The docs.rs index opened with about forty lines of raw `pub use` and no
prose at all — nothing saying what the crate is or how to make one call.

- a crate-level `//!`: what it is, a runnable `create_api_service()`
  example, a table mapping each `ApiService` field to its module, then the
  four things a caller hits immediately — `DataWrapper`/`GraphDataWrapper`,
  id-or-externalId, `ResponseError` and its problem document, and the
  list/filter/search split.
- `[package.metadata.docs.rs] all-features = true`, so the `blocking`
  client is documented at all. docs.rs builds default-features-only, so a
  feature the README points callers at was absent from the reference.
  `doc_cfg` badges it, guarded by `cfg_attr(docsrs, ...)` so stable builds
  are unaffected.
- clears all five rustdoc warnings. Two were links into private items,
  which render as dead text rather than links: `DEFAULT_SCOPE` pointing at
  `OAuthConfig::effective_scope`, and `set_advanced_filter` at a private
  field. Both now say the thing instead of linking to it.

Verified clean on stable and on nightly with `--cfg docsrs`, which is what
docs.rs runs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: jgjesdal <jostein@intellistream.ai>
@JosteinGj @claude
docs: document ApiService and create_api_service
88243b5 
The entry point and the type every caller starts from both had no docs, so
the two most-visited pages in the reference were a bare signature and a list
of bare field names.

`ApiService` now says what it is, that the services share one HTTP client
and token cache so the Arc should be cloned rather than the client rebuilt,
and that one client is one tenant. Each service field links to its module.

`create_api_service` documents what it reads from the environment, carries a
runnable example, and has a Panics section — it calls `from_env().unwrap()`,
so a missing BASE_URL panics at construction rather than on the first call,
and `DataHubConfig::from_env` + `ApiService::new` is the Result-based route.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: jgjesdal <jostein@intellistream.ai>
@JosteinGj @claude
docs: describe every module, and hide the plumbing from the reference
91629d3 
Nineteen of the twenty-three modules rendered on the docs.rs index as a
bare name with no description, including every service a caller starts
from. Each now says what it is, what it covers, and the one or two things
that will otherwise surprise you — events.list() returning the oldest
events, Function::related_resources always being empty, FileUpload::new
panicking on an unreadable path, a listener closing as idle if it is not
polled.

Hides four items that are public only because the services need them, as
one group so no survivor is left referencing a hidden name:
ApiServiceProvider, DataWrapperDeserialization, process_response and the
serde_helper module. doc(hidden) rather than pub(crate) — serde_helper's
id-as-string adapters are legitimately reusable by a downstream type's
own #[serde(with = ...)], and hiding does not restrict.

GraphNode and Identifiable stay visible deliberately: both appear as
bounds in public signatures, where hiding renders them as unclickable
bare names.

Four doc comments contradicted the api and are corrected here:
- labels::get said a miss returns empty items; it is a 404.
- EdgeProxy claimed it is returned from GET /functions, which answers
  RelatedNode and never populates it.
- TimeSeries::value_type said graph reads do not carry it; they do now.
- the subscriptions module is not "CRUD" — there is no update or get.

labels::delete's documented 400 was checked against the backend and left
alone: it really is a 400 with `fields`, not the 409 `would-strand` that
resources and edges answer with.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: jgjesdal <jostein@intellistream.ai>
@JosteinGj @claude
docs: describe every public type
3e6d36f 
Thirty-four public types rendered with no description at all, including the
ones a caller meets first: DataWrapper, ResponseError, Event, TimeSeries,
Resource, Dataset, Unit, Subscription and every service struct.

Service structs get a one-liner pointing at their module doc rather than
repeating it. Entity types say what the thing is and carry the one fact
that is not guessable from the field list — an Event is keyed by UUID and
its externalId is deliberately not unique; a Dataset's connected_data_sets
is input-only; a TimeSeries value_type cannot change after creation;
DatapointString is stringly-typed so a decimal never passes through an f64;
FileUpload::new panics on an unreadable path.

DataWrapper gets the longest entry, since it is the return type of nearly
every call and the "it is also a request body" trick is what makes
`create(&event)` work.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: jgjesdal <jostein@intellistream.ai>
@JosteinGj @claude
docs: document the remaining service methods
167aa4d 
Twenty-eight API calls still had no doc comment, so the reference showed a
bare signature for things like datasets.create, events.filter,
files.upload_file, timeseries.retrieve_datapoints and the whole units
service.

Each now names the endpoint it calls and carries what the signature cannot
say: that events.create stamps a UUID v7 so a retry dedups and can answer
202 when buffering spooled the batch; that resources.delete is refused with
409 would-strand; that files.delete is a soft delete; that
retrieve_datapoints takes a half-open window; that a supplied TOKEN is
never refreshed; that deleting a subscription with a live listener attached
is refused.

Leaves the builder setters and field getters alone. A one-line doc on
`set_name` saying it sets the name is noise, and there are several hundred
of them; the types they hang off are documented instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: jgjesdal <jostein@intellistream.ai>
@JosteinGj @claude
ci: fail the build when the docs break
7522e16 
Nothing was checking the documentation, so the two things that make a
reference actively wrong both happened silently.

A broken intra-doc link is not a build failure — rustdoc renders it as
dead text and carries on, so it only shows up on docs.rs, after release.
There were five in the tree before this branch. `-D warnings` on the
rustdoc build turns them into failures.

And the examples were never compiled: CI runs `cargo test --no-run`, which
does not build doctests at all. So the `create_api_service()` example on
the landing page would have rotted the moment a method was renamed, with
nothing to say so.

Only `--doc` is run here. Every other test in this crate needs a live
backend, which CI has no way to reach; the doctests are the documentation's
own examples and are pure construction, verified to pass with BASE_URL
pointed at a dead address and no credentials in the environment.

The doc build uses nightly with `--cfg docsrs` to match what docs.rs
actually runs, since `doc_cfg` — which badges the `blocking` feature — is
nightly-only.

Verified by injecting a broken link: the step exits 101.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: jgjesdal <jostein@intellistream.ai>
@JosteinGj
JosteinGj force-pushed the chore/remove-dead-code branch from c4507ec to 5cf32f7 Compare September 24, 2026 09:27
Base automatically changed from docs/correct-api-mismatches to main September 24, 2026 09:51
JosteinGj and others added 2 commits September 24, 2026 14:48
@JosteinGj @claude
docs: build the reference on stable, and say in the blocking module d…
5fdbb71 
…oc that it needs the feature

The doc_cfg badge on `blocking` was the only reason for nightly, in CI and in the docs.rs
rustdoc-args. The module doc now names the feature and shows the Cargo.toml line instead.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: jgjesdal <jostein@intellistream.ai>
@JosteinGj @claude
ci: run the doctests in the crate job instead of a separate docs job
f0271fc 
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: jgjesdal <jostein@intellistream.ai>
@olavgg

olavgg commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

Should this be reviewed?

@olavgg
Merge pull request #148 from IntelliStream-DataHub/refactor/remove-in…
f44f4e5 
…ert-config-and-auth-diagnostics

refactor!: remove the inert PROJECT_NAME and the auth_diagnostics module
@olavgg
Merge pull request #149 from IntelliStream-DataHub/docs/usable-api-re…
3bd047f 
…ference

docs: make the docs.rs reference usable
@JosteinGj JosteinGj closed this Oct 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@JosteinGj @olavgg