Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix doc warnings #2305

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix doc warnings #2305

wants to merge 1 commit into from

Conversation

mamcx
Copy link
Contributor

@mamcx mamcx commented Feb 25, 2025

Description of Changes

Fix as many warnings caused by cargo doc.

Some that stay:

  • Of the form warning: public documentation for Xlinks to private itemY`
  • The top doc on crates/core/src/subscription/subscription.rs This generate unresolved link warnings but looks to me that need a full rewrite c.c. @joshua-spacetime.

Expected complexity level and risk

0

Testing

  • Run cargo test --all. Because we have a naming collision:
rror: document output filename collision
The lib `spacetimedb` in package `spacetimedb-core v1.0.0-rc4 (.../space/SpacetimeDB/crates/core)` has the same name as the lib `spacetimedb` in package `spacetimedb v1.0.0-rc4 (../space/SpacetimeDB/crates/bindings)`.
Only one may be documented at once since they output to the same path.

... then I switch adding doc =false in each to let me see all the warnings.

@mamcx mamcx added documentation Improvements or additions to documentation release-any To be landed in any release window labels Feb 25, 2025
@mamcx mamcx self-assigned this Feb 25, 2025
mamcx
mamcx commented Feb 25, 2025
View reviewed changes
crates/core/src/db/datastore/system_tables.rs
@@ -8,7 +8,7 @@
//! - You will probably need to add a new ID type in `spacetimedb_primitives`,
//! with trait implementations in `spacetimedb_sats::{typespace, de::impl, ser::impl}`.
//! - Add it to [`system_tables`], and define a constant for its index there.
//! - Use [`st_fields_enum`] to define its column enum.
//! - Use `st_fields_enum` to define its column enum.
//! - Register its schema in [`system_module_def`], making sure to call `validate_system_table` at the end of the function.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly st_fields_enum is not linkable here

crates/core/src/host/wasmtime/wasm_instance_env.rs
@@ -1107,8 +1107,6 @@ impl WasmInstanceEnv {
/// - `target` is not NULL and `target_ptr[..target_len]` is not in bounds of WASM memory.
/// - `filename` is not NULL and `filename_ptr[..filename_len]` is not in bounds of WASM memory.
/// - `message` is not NULL and `message_ptr[..message_len]` is not in bounds of WASM memory.
///
/// [target]: https://docs.rs/log/latest/log/struct.Record.html#method.target
#[tracing::instrument(level = "trace", skip_all)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not understood by cargo doc

crates/core/src/subscription/execution_unit.rs
@@ -88,7 +88,7 @@ pub struct ExecutionUnit {
/// This is a direct compilation of the source query.
eval_plan: QueryExpr,
/// A version of the plan optimized for `eval_incr`,
/// whose source is an in-memory table, as if by [`query::to_mem_table`].
/// whose source is an in-memory table.
eval_incr_plan: EvalIncrPlan,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to_mem_table is now in [test]

crates/core/src/subscription/query.rs
@@ -25,7 +25,7 @@ pub const SUBSCRIBE_TO_ALL_QUERY: &str = "SELECT * FROM *";
/// rather than returning a new `SourceSet`.
///
/// This is necessary when merging multiple SQL queries into a single query set,
/// as in [`crate::subscription::module_subscription_actor::ModuleSubscriptions::add_subscriber`].
/// as in [`crate::subscription::module_subscription_actor::ModuleSubscriptions::add_multi_subscription`].
pub fn compile_read_only_queryset(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I not sure if this is correct.

crates/sdk/tests/connect_disconnect_client/src/module_bindings/mod.rs
@@ -535,7 +535,7 @@ impl __sdk::DbContext for ReducerEventContext {

impl __sdk::ReducerEventContext for ReducerEventContext {}

/// An [`__sdk::DbContext`] passed to [`__sdk::SubscriptionBuilder::on_applied`] and [`SubscriptionHandle::unsubscribe_then`] callbacks.
/// An [`__sdk::DbContext`] passed to subscription callbacks.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many more now.

crates/sqltest/src/main.rs
@@ -327,7 +327,7 @@ async fn update_test_files(files: Vec<PathBuf>, engine: DbType, format: bool) ->
Ok(())
}

/// Different from [`sqllogictest::update_test_file`], we re-implement it here to print some
/// Different from [`sqllogictest`], we re-implement logic here to print some
/// progress information.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original impl is not there, but this is fine, this only internal and nobody else use it.

crates/vm/src/expr.rs
@@ -620,8 +620,6 @@ impl SourceExpr {
/// If `self` refers to a [`DbTable`], get a reference to it.
///
/// Returns `None` if `self` refers to a [`MemTable`].
/// In that case, retrieving the [`MemTable`] requires inspecting the plan's corresponding [`SourceSet`]
/// via [`SourceSet::take_mem_table`] or [`SourceSet::take_table`].
pub fn get_db_table(&self) -> Option<&DbTable> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't see how rewrite this part...

crates/vm/src/lib.rs
//!
//! Run the AST build from [expr::Expr]. It assumes is correct.
//!

pub use spacetimedb_lib::operator;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our vm is not a vm anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bfops bfops Awaiting requested review from bfops bfops is a code owner

@cloutiertyler cloutiertyler Awaiting requested review from cloutiertyler cloutiertyler is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@mamcx mamcx

Labels
documentation Improvements or additions to documentation release-any To be landed in any release window
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@mamcx