-
-
Notifications
You must be signed in to change notification settings - Fork 250
Class Docs – register docs in #[godot_api(secondary)]
, simplify docs registration logic
#1355
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.
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
base: master
Are you sure you want to change the base?
Conversation
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1355 |
b37c2c8
to
4830461
Compare
#[godot_api(secondary)]
, simplify registration logic#[godot_api(secondary)]
, simplify docs registration logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!
A big part of code has been deleted + re-added, Git won't detect as a move. I guess it's not easy to split this so that that's possible? (If not, that's fine, don't spend too much time).
/// You should not manually construct this struct, but rather use [`DocsPlugin::new()`]. | ||
#[derive(Debug)] | ||
pub struct DocsPlugin { | ||
/// The name of the class to register docs for. | ||
pub(crate) class_name: ClassId, | ||
|
||
/// The actual item being registered. | ||
pub item: DocsItem, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question on comment and field visibility: why is one pub(crate)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover after copy-pasting ClassPlugin
(and extracting proper constructors).
godot-core/src/docs.rs
Outdated
pub enum DocsItem { | ||
Struct(StructDocs), | ||
InherentImpl(InherentImplDocs), | ||
VirtualMethods(&'static str), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to add a TraitImpl(TraitImplDocs)
already, for consistency with the other enum variants? Also because we'd have a named field 🤔
godot-macros/src/docs.rs
Outdated
* 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/. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this to docs/mod.rs
, I try not to use the other module style 🧐
godot-macros/src/docs.rs
Outdated
/// XML attribute, as BBCode: `deprecated="DEPRECATED"`. | ||
/// Contains whole paragraph annotated with a `@deprecated` tag. | ||
deprecated_attr: String, | ||
#[cfg(not(all(feature = "register-docs", since_api = "4.3")))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do wonder if we can simplify those to just check register-docs
, as we already verify (AFAIR?) the compatibility with api-*
features?
Maybe in separate PR after this is merged, so there's not too many changes at once.
4830461
to
9d302af
Compare
e34b423
to
e1ed40d
Compare
- Bugfix: Handle docs in `#[godot_api(secondary)]` - Code tweaks: Separate docs modules to not pollute rest of the code with `#[cfg(all(feature = "register-docs", since_api = "4.3"))]`. Simplify registration logic. - Create separate PluginRegistry for Docs and Docs only.
e1ed40d
to
e1b84ba
Compare
What problem does this PR solve?
The obvious one – #1354, additionally I finally decided to address cfg leaking all over the codebase – i.e. handling docs-related cfg attr and features happens in docs module, not… everywhere else 😅.
I created another plugin registry for docs alone; initial logic by @Houtamelo directly edited plugin item with
InherentImpl
but I don't see it being maintainable (i.e. it is error prone + will devolve into chaos + mixing these boundaries is bad idea in general).I'm not sure if additional registry is the best idea but I couldn't find anything better 🤷 and it is still gated behind
register-docs
feature.I'm marking it as a feature, despite being an oversight 🧐.
Doc bugfixes and improvements
#[godot_api(secondary)]
#[cfg(all(feature = "register-docs", since_api = "4.3"))]
. Simplify registration logic.