From 0af7118f6919981b9a10c6fab7036b0a4358d7ed Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sun, 3 Nov 2024 23:06:40 +0100 Subject: [PATCH 1/5] Enable `register-docs` feature for unit test/clippy --- .github/workflows/full-ci.yml | 10 ++++++---- .github/workflows/minimal-ci.yml | 11 ++++++----- godot/tests/docs.rs | 4 +++- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/.github/workflows/full-ci.yml b/.github/workflows/full-ci.yml index 2d32c2694..cace6dc8d 100644 --- a/.github/workflows/full-ci.yml +++ b/.github/workflows/full-ci.yml @@ -13,7 +13,9 @@ on: env: - GDEXT_FEATURES: '' + # Applies to all 'register-docs' features across crates. + CLIPPY_FEATURES: '--features register-docs' + TEST_FEATURES: '' RETRY: ${{ github.workspace }}/.github/other/retry.sh # ASan options: https://github.com/google/sanitizers/wiki/AddressSanitizerFlags @@ -99,7 +101,7 @@ jobs: # Note: could use `-- --no-deps` to not lint dependencies, however it doesn't really speed up and also skips deps in workspace. - name: "Check clippy" run: | - cargo clippy --all-targets $GDEXT_FEATURES -- \ + cargo clippy --all-targets $CLIPPY_FEATURES -- \ -D clippy::suspicious \ -D clippy::style \ -D clippy::complexity \ @@ -161,10 +163,10 @@ jobs: run: cargo +nightly update -Z minimal-versions - name: "Compile tests" - run: cargo test $GDEXT_FEATURES --no-run ${{ matrix.rust-extra-args }} + run: cargo test $TEST_FEATURES --no-run ${{ matrix.rust-extra-args }} - name: "Test" - run: cargo test $GDEXT_FEATURES ${{ matrix.rust-extra-args }} + run: cargo test $TEST_FEATURES ${{ matrix.rust-extra-args }} miri-test: diff --git a/.github/workflows/minimal-ci.yml b/.github/workflows/minimal-ci.yml index 216e3d188..c815d0de0 100644 --- a/.github/workflows/minimal-ci.yml +++ b/.github/workflows/minimal-ci.yml @@ -21,8 +21,9 @@ on: env: - GDEXT_FEATURES: '' - # GDEXT_FEATURES: '--features crate/feature' + # Applies to all 'register-docs' features across crates. + CLIPPY_FEATURES: '--features register-docs' + TEST_FEATURES: '' # GDEXT_CRATE_ARGS: '-p godot-codegen -p godot-ffi -p godot-core -p godot-macros -p godot' RETRY: ${{ github.workspace }}/.github/other/retry.sh @@ -96,7 +97,7 @@ jobs: - name: "Check clippy" run: | - cargo clippy --all-targets $GDEXT_FEATURES -- \ + cargo clippy --all-targets $CLIPPY_FEATURES -- \ -D clippy::suspicious \ -D clippy::style \ -D clippy::complexity \ @@ -120,10 +121,10 @@ jobs: uses: ./.github/composite/rust - name: "Compile tests" - run: cargo test $GDEXT_FEATURES --no-run + run: cargo test $TEST_FEATURES --no-run - name: "Test" - run: cargo test $GDEXT_FEATURES + run: cargo test $TEST_FEATURES # For complex matrix workflow, see https://stackoverflow.com/a/65434401 diff --git a/godot/tests/docs.rs b/godot/tests/docs.rs index 78fe3b5d8..a3d572ab9 100644 --- a/godot/tests/docs.rs +++ b/godot/tests/docs.rs @@ -1,10 +1,12 @@ -#![cfg(feature = "register-docs")] /* * Copyright (c) godot-rust; Bromeon and contributors. * This Source Code Form is subject to the terms of the Mozilla Public * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#![cfg(feature = "register-docs")] + use godot::prelude::*; /// *documented* ~ **documented** ~ [AABB] < [pr](https://github.com/godot-rust/gdext/pull/748) From b6a8779cc56de02e161fc1ee7f265bb6f035a1f5 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sun, 3 Nov 2024 23:11:01 +0100 Subject: [PATCH 2/5] Fix Clippy issues in `register-docs` gated code --- godot-core/src/docs.rs | 1 + godot-macros/src/docs.rs | 6 +++--- godot-macros/src/docs/markdown_converter.rs | 2 +- itest/rust/src/register_tests/constant_test.rs | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/godot-core/src/docs.rs b/godot-core/src/docs.rs index 9e153f254..f839895de 100644 --- a/godot-core/src/docs.rs +++ b/godot-core/src/docs.rs @@ -89,6 +89,7 @@ pub fn gather_xml_docs() -> impl Iterator { PluginItem::Struct { docs: Some(docs), .. } => map.entry(class_name).or_default().definition = docs, + _ => (), } }); diff --git a/godot-macros/src/docs.rs b/godot-macros/src/docs.rs index 14876984f..abe967862 100644 --- a/godot-macros/src/docs.rs +++ b/godot-macros/src/docs.rs @@ -21,7 +21,7 @@ pub fn make_definition_docs( let base_escaped = xml_escape(base); let desc_escaped = xml_escape(make_docs_from_attributes(description)?); let members = members - .into_iter() + .iter() .filter(|x| x.var.is_some() | x.export.is_some()) .filter_map(member) .collect::(); @@ -112,7 +112,7 @@ fn siphon_docs_from_attributes(doc: &[Attribute]) -> impl Iterator None, }) .flat_map(|doc| { - doc.into_iter().map(|x| { + doc.iter().map(|x| { x.to_string() .trim_start_matches('r') .trim_start_matches('#') @@ -126,7 +126,7 @@ fn siphon_docs_from_attributes(doc: &[Attribute]) -> impl Iterator String { // Most strings have no special characters, so this check helps avoid unnecessary string copying - if !value.contains(&['&', '<', '>', '"', '\'']) { + if !value.contains(['&', '<', '>', '"', '\'']) { return value; } diff --git a/godot-macros/src/docs/markdown_converter.rs b/godot-macros/src/docs/markdown_converter.rs index 7b93a2236..e468a82ae 100644 --- a/godot-macros/src/docs/markdown_converter.rs +++ b/godot-macros/src/docs/markdown_converter.rs @@ -79,7 +79,7 @@ fn walk_node(node: &md::Node, definitions: &HashMap<&str, &str>) -> Option html.value.clone(), - _ => walk_nodes(&node.children()?, definitions, ""), + _ => walk_nodes(node.children()?, definitions, ""), }; Some(bbcode) diff --git a/itest/rust/src/register_tests/constant_test.rs b/itest/rust/src/register_tests/constant_test.rs index 6fa0ad7dd..be51c7f3d 100644 --- a/itest/rust/src/register_tests/constant_test.rs +++ b/itest/rust/src/register_tests/constant_test.rs @@ -174,7 +174,7 @@ godot::sys::plugin_add!( raw: ::godot::private::callbacks::register_user_methods_constants::, }, register_rpcs_fn: None, - #[cfg(all(since_api = "4.3", feature = "register-docs"))] + #[cfg(feature = "register-docs")] docs: ::godot::docs::InherentImplDocs::default(), }), init_level: HasOtherConstants::INIT_LEVEL, From 927e5f8a6f66540c0d9ee3ec7d940167c1a8c5d7 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Mon, 4 Nov 2024 20:42:50 +0100 Subject: [PATCH 3/5] Move register-docs test to itest suite --- itest/rust/src/register_tests/mod.rs | 1 + .../rust/src/register_tests/register_docs_test.rs | 12 +++++++----- .../rust/src/register_tests/res/registered_docs.xml | 0 3 files changed, 8 insertions(+), 5 deletions(-) rename godot/tests/docs.rs => itest/rust/src/register_tests/register_docs_test.rs (94%) rename godot/tests/test_data/docs.xml => itest/rust/src/register_tests/res/registered_docs.xml (100%) diff --git a/itest/rust/src/register_tests/mod.rs b/itest/rust/src/register_tests/mod.rs index 14c586622..5b7d225c5 100644 --- a/itest/rust/src/register_tests/mod.rs +++ b/itest/rust/src/register_tests/mod.rs @@ -12,6 +12,7 @@ mod func_test; mod gdscript_ffi_test; mod naming_tests; mod option_ffi_test; +mod register_docs_test; #[cfg(feature = "codegen-full")] mod rpc_test; mod var_test; diff --git a/godot/tests/docs.rs b/itest/rust/src/register_tests/register_docs_test.rs similarity index 94% rename from godot/tests/docs.rs rename to itest/rust/src/register_tests/register_docs_test.rs index a3d572ab9..177c0af7b 100644 --- a/godot/tests/docs.rs +++ b/itest/rust/src/register_tests/register_docs_test.rs @@ -7,6 +7,7 @@ #![cfg(feature = "register-docs")] +use crate::framework::itest; use godot::prelude::*; /// *documented* ~ **documented** ~ [AABB] < [pr](https://github.com/godot-rust/gdext/pull/748) @@ -189,15 +190,16 @@ impl FairlyDocumented { fn documented_signal(p: Vector3, w: f64, node: Gd); } -#[test] -fn correct() { +#[itest(focus)] +fn test_register_docs() { // Uncomment if implementation changes and expected output file should be rewritten. // std::fs::write( - // "tests/test_data/docs.xml", + // "res/registered_docs.xml", // godot_core::docs::gather_xml_docs().next().unwrap(), // ); + assert_eq!( - include_str!("test_data/docs.xml"), - godot_core::docs::gather_xml_docs().next().unwrap() + include_str!("res/registered_docs.xml"), + godot::docs::gather_xml_docs().next().unwrap() ); } diff --git a/godot/tests/test_data/docs.xml b/itest/rust/src/register_tests/res/registered_docs.xml similarity index 100% rename from godot/tests/test_data/docs.xml rename to itest/rust/src/register_tests/res/registered_docs.xml From 650c2ad784c1e1c7a8dca7bd87e80e2b534f6f41 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Mon, 4 Nov 2024 21:20:52 +0100 Subject: [PATCH 4/5] Update register-docs tests so they pass again --- itest/rust/Cargo.toml | 2 +- .../rust/src/register_tests/constant_test.rs | 2 +- .../src/register_tests/register_docs_test.rs | 29 ++++++++++++------- .../register_tests/res/registered_docs.xml | 2 +- 4 files changed, 22 insertions(+), 13 deletions(-) diff --git a/itest/rust/Cargo.toml b/itest/rust/Cargo.toml index ce3a754c3..28516f1f8 100644 --- a/itest/rust/Cargo.toml +++ b/itest/rust/Cargo.toml @@ -14,7 +14,7 @@ default = ["codegen-full"] codegen-full = ["godot/__codegen-full"] codegen-full-experimental = ["codegen-full", "godot/experimental-godot-api"] experimental-threads = ["godot/experimental-threads"] -register-docs = ["godot/register-docs"] # TODO remove as soon as constant_test.rs checks bitfields with #[constant] proc-macro +register-docs = ["godot/register-docs"] serde = ["dep:serde", "dep:serde_json", "godot/serde"] # Do not add features here that are 1:1 forwarded to the `godot` crate, unless they are needed by itest itself. diff --git a/itest/rust/src/register_tests/constant_test.rs b/itest/rust/src/register_tests/constant_test.rs index be51c7f3d..33af9ce12 100644 --- a/itest/rust/src/register_tests/constant_test.rs +++ b/itest/rust/src/register_tests/constant_test.rs @@ -164,7 +164,7 @@ impl godot::obj::cap::ImplementsGodotApi for HasOtherConstants { } } -// TODO once this is done via proc-macro, remove `register-docs` feature from itest, and update CI workflows. +// TODO once this is done via proc-macro, see if `register-docs` is still used in register_docs_test.rs. Otherwise, remove feature from Cargo.toml. godot::sys::plugin_add!( __GODOT_PLUGIN_REGISTRY in ::godot::private; ::godot::private::ClassPlugin { diff --git a/itest/rust/src/register_tests/register_docs_test.rs b/itest/rust/src/register_tests/register_docs_test.rs index 177c0af7b..848e9ab95 100644 --- a/itest/rust/src/register_tests/register_docs_test.rs +++ b/itest/rust/src/register_tests/register_docs_test.rs @@ -190,16 +190,25 @@ impl FairlyDocumented { fn documented_signal(p: Vector3, w: f64, node: Gd); } -#[itest(focus)] +#[itest] fn test_register_docs() { + let xml = find_class_docs("FairlyDocumented"); + // Uncomment if implementation changes and expected output file should be rewritten. - // std::fs::write( - // "res/registered_docs.xml", - // godot_core::docs::gather_xml_docs().next().unwrap(), - // ); - - assert_eq!( - include_str!("res/registered_docs.xml"), - godot::docs::gather_xml_docs().next().unwrap() - ); + // std::fs::write("../rust/src/register_tests/res/registered_docs.xml", &xml) + // .expect("failed to write docs XML file"); + + assert_eq!(include_str!("res/registered_docs.xml"), xml); +} + +fn find_class_docs(class_name: &str) -> String { + let mut count = 0; + for xml in godot::docs::gather_xml_docs() { + count += 1; + if xml.contains(class_name) { + return xml; + } + } + + panic!("Registered docs for class {class_name} not found in {count} XML files"); } diff --git a/itest/rust/src/register_tests/res/registered_docs.xml b/itest/rust/src/register_tests/res/registered_docs.xml index ea575dd34..55eefd5f8 100644 --- a/itest/rust/src/register_tests/res/registered_docs.xml +++ b/itest/rust/src/register_tests/res/registered_docs.xml @@ -25,7 +25,7 @@ these - + From fba9f61fe6af725f742653b0b33089294127a976 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Mon, 4 Nov 2024 23:19:47 +0100 Subject: [PATCH 5/5] Consistently name all related features `register-docs` --- godot-core/Cargo.toml | 2 +- godot-core/src/init/mod.rs | 2 +- godot-core/src/lib.rs | 6 +++--- godot-core/src/registry/class.rs | 6 +++--- godot-core/src/registry/plugin.rs | 8 ++++---- godot-macros/Cargo.toml | 2 +- godot-macros/src/class/data_models/field.rs | 4 ++-- godot-macros/src/class/data_models/inherent_impl.rs | 4 ++-- .../src/class/data_models/interface_trait_impl.rs | 4 ++-- godot-macros/src/class/derive_godot_class.rs | 4 ++-- godot-macros/src/lib.rs | 2 +- godot/Cargo.toml | 2 +- 12 files changed, 23 insertions(+), 23 deletions(-) diff --git a/godot-core/Cargo.toml b/godot-core/Cargo.toml index 61f86213b..8b15f1ff7 100644 --- a/godot-core/Cargo.toml +++ b/godot-core/Cargo.toml @@ -12,7 +12,7 @@ homepage = "https://godot-rust.github.io" [features] default = [] -docs = [] +register-docs = [] codegen-rustfmt = ["godot-ffi/codegen-rustfmt", "godot-codegen/codegen-rustfmt"] codegen-full = ["godot-codegen/codegen-full"] codegen-lazy-fptrs = [ diff --git a/godot-core/src/init/mod.rs b/godot-core/src/init/mod.rs index df94b18dd..5a287b0a5 100644 --- a/godot-core/src/init/mod.rs +++ b/godot-core/src/init/mod.rs @@ -143,7 +143,7 @@ unsafe fn gdext_on_level_init(level: InitLevel) { unsafe { ensure_godot_features_compatible() }; } InitLevel::Editor => { - #[cfg(all(since_api = "4.3", feature = "docs"))] + #[cfg(all(since_api = "4.3", feature = "register-docs"))] // SAFETY: Godot binding is initialized, and this is called from the main thread. unsafe { crate::docs::register(); diff --git a/godot-core/src/lib.rs b/godot-core/src/lib.rs index e32a4b7e4..567d1258e 100644 --- a/godot-core/src/lib.rs +++ b/godot-core/src/lib.rs @@ -15,11 +15,11 @@ pub mod builder; pub mod builtin; pub mod classes; -#[cfg(all(since_api = "4.3", feature = "docs"))] +#[cfg(all(since_api = "4.3", feature = "register-docs"))] pub mod docs; #[doc(hidden)] pub mod possibly_docs { - #[cfg(all(since_api = "4.3", feature = "docs"))] + #[cfg(all(since_api = "4.3", feature = "register-docs"))] pub use crate::docs::*; } pub mod global; @@ -35,7 +35,7 @@ pub use godot_ffi as sys; // ---------------------------------------------------------------------------------------------------------------------------------------------- // Validations (see also godot/lib.rs) -#[cfg(all(feature = "docs", before_api = "4.3"))] +#[cfg(all(feature = "register-docs", before_api = "4.3"))] compile_error!("Generating editor docs for Rust symbols requires at least Godot 4.3."); // ---------------------------------------------------------------------------------------------------------------------------------------------- diff --git a/godot-core/src/registry/class.rs b/godot-core/src/registry/class.rs index 0a51785fb..678781b36 100644 --- a/godot-core/src/registry/class.rs +++ b/godot-core/src/registry/class.rs @@ -243,7 +243,7 @@ fn fill_class_info(item: PluginItem, c: &mut ClassRegistrationInfo) { is_editor_plugin, is_internal, is_instantiable, - #[cfg(all(since_api = "4.3", feature = "docs"))] + #[cfg(all(since_api = "4.3", feature = "register-docs"))] docs: _, } => { c.parent_class_name = Some(base_class_name); @@ -296,7 +296,7 @@ fn fill_class_info(item: PluginItem, c: &mut ClassRegistrationInfo) { PluginItem::InherentImpl(InherentImpl { register_methods_constants_fn, register_rpcs_fn: _, - #[cfg(all(since_api = "4.3", feature = "docs"))] + #[cfg(all(since_api = "4.3", feature = "register-docs"))] docs: _, }) => { c.register_methods_constants_fn = Some(register_methods_constants_fn); @@ -315,7 +315,7 @@ fn fill_class_info(item: PluginItem, c: &mut ClassRegistrationInfo) { user_free_property_list_fn, user_property_can_revert_fn, user_property_get_revert_fn, - #[cfg(all(since_api = "4.3", feature = "docs"))] + #[cfg(all(since_api = "4.3", feature = "register-docs"))] virtual_method_docs: _, } => { c.user_register_fn = user_register_fn; diff --git a/godot-core/src/registry/plugin.rs b/godot-core/src/registry/plugin.rs index 55971fcbc..db191c774 100644 --- a/godot-core/src/registry/plugin.rs +++ b/godot-core/src/registry/plugin.rs @@ -5,7 +5,7 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -#[cfg(all(since_api = "4.3", feature = "docs"))] +#[cfg(all(since_api = "4.3", feature = "register-docs"))] use crate::docs::*; use crate::init::InitLevel; use crate::meta::ClassName; @@ -65,7 +65,7 @@ pub struct InherentImpl { /// /// This function is called in [`UserClass::__before_ready()`](crate::obj::UserClass::__before_ready) definitions generated by the `#[derive(GodotClass)]` macro. pub register_rpcs_fn: Option, - #[cfg(all(since_api = "4.3", feature = "docs"))] + #[cfg(all(since_api = "4.3", feature = "register-docs"))] pub docs: InherentImplDocs, } @@ -121,7 +121,7 @@ pub enum PluginItem { /// Whether the class has a default constructor. is_instantiable: bool, - #[cfg(all(since_api = "4.3", feature = "docs"))] + #[cfg(all(since_api = "4.3", feature = "register-docs"))] docs: Option, }, @@ -130,7 +130,7 @@ pub enum PluginItem { /// Collected from `#[godot_api] impl I... for MyClass`. ITraitImpl { - #[cfg(all(since_api = "4.3", feature = "docs"))] + #[cfg(all(since_api = "4.3", feature = "register-docs"))] /// Virtual method documentation. virtual_method_docs: &'static str, /// Callback to user-defined `register_class` function. diff --git a/godot-macros/Cargo.toml b/godot-macros/Cargo.toml index b5df22634..23fa5769b 100644 --- a/godot-macros/Cargo.toml +++ b/godot-macros/Cargo.toml @@ -12,7 +12,7 @@ homepage = "https://godot-rust.github.io" [features] api-custom = ["godot-bindings/api-custom"] -docs = ["dep:markdown"] +register-docs = ["dep:markdown"] codegen-full = ["godot/__codegen-full"] [lib] diff --git a/godot-macros/src/class/data_models/field.rs b/godot-macros/src/class/data_models/field.rs index 28173df0e..0924fae09 100644 --- a/godot-macros/src/class/data_models/field.rs +++ b/godot-macros/src/class/data_models/field.rs @@ -15,7 +15,7 @@ pub struct Field { pub var: Option, pub export: Option, pub is_onready: bool, - #[cfg(feature = "docs")] + #[cfg(feature = "register-docs")] pub attributes: Vec, } @@ -28,7 +28,7 @@ impl Field { var: None, export: None, is_onready: false, - #[cfg(feature = "docs")] + #[cfg(feature = "register-docs")] attributes: field.attributes.clone(), } } diff --git a/godot-macros/src/class/data_models/inherent_impl.rs b/godot-macros/src/class/data_models/inherent_impl.rs index 41b60dd3e..245f26ccc 100644 --- a/godot-macros/src/class/data_models/inherent_impl.rs +++ b/godot-macros/src/class/data_models/inherent_impl.rs @@ -74,9 +74,9 @@ pub fn transform_inherent_impl(mut impl_block: venial::Impl) -> ParseResult ParseResult ParseResult { let is_internal = struct_cfg.is_internal; let base_ty = &struct_cfg.base_ty; - #[cfg(all(feature = "docs", since_api = "4.3"))] + #[cfg(all(feature = "register-docs", since_api = "4.3"))] let docs = crate::docs::make_definition_docs( base_ty.to_string(), &class.attributes, &fields.all_fields, ); - #[cfg(not(all(feature = "docs", since_api = "4.3")))] + #[cfg(not(all(feature = "register-docs", since_api = "4.3")))] let docs = quote! {}; let base_class = quote! { ::godot::classes::#base_ty }; let base_class_name_obj = util::class_name_obj(&base_class); diff --git a/godot-macros/src/lib.rs b/godot-macros/src/lib.rs index dd3e2a74a..05e88843e 100644 --- a/godot-macros/src/lib.rs +++ b/godot-macros/src/lib.rs @@ -13,7 +13,7 @@ mod bench; mod class; mod derive; -#[cfg(all(feature = "docs", since_api = "4.3"))] +#[cfg(all(feature = "register-docs", since_api = "4.3"))] mod docs; mod gdextension; mod itest; diff --git a/godot/Cargo.toml b/godot/Cargo.toml index e3aa46a6a..70e678470 100644 --- a/godot/Cargo.toml +++ b/godot/Cargo.toml @@ -24,7 +24,7 @@ codegen-rustfmt = ["godot-core/codegen-rustfmt"] lazy-function-tables = ["godot-core/codegen-lazy-fptrs"] serde = ["godot-core/serde"] -register-docs = ["godot-macros/docs", "godot-core/docs"] +register-docs = ["godot-macros/register-docs", "godot-core/register-docs"] api-custom = ["godot-core/api-custom"] # [version-sync] [[