Skip to content

Conversation

@GuillaumeGomez
Copy link
Contributor

While looking at some examples, I realized that it needed some features to be enabled because I couldn't find the related docs I built locally. It doesn't solve this issue unfortunately but at least when people will look on docs.rs, they will be able to find these items.

@mockersf
Copy link
Member

Some of the features you added are already present in the default feature set:

bevy/Cargo.toml

Lines 27 to 40 in e96b21a

default = [
"animation",
"bevy_asset",
"bevy_audio",
"bevy_gilrs",
"bevy_scene",
"bevy_winit",
"render",
"png",
"hdr",
"vorbis",
"x11",
"filesystem_watcher",
]

Doesn't docs.rs use those? Otherwise https://docs.rs/bevy/latest/bevy/animation/struct.AnimationClip.html shouldn't be present as it's behind the animation feature

@mockersf mockersf added the C-Docs An addition or correction to our documentation label Sep 17, 2022
@Nilirad
Copy link
Contributor

Nilirad commented Sep 17, 2022

Hmm, maybe we should enable all features?

@GuillaumeGomez
Copy link
Contributor Author

I don't know which ones are for internal use (ie bevy development) and which ones are to be used by users. If you have a list of features that shouldn't appear in the docs, I will update the PR.

Some of the features you added are already present in the default feature set:

Indeed, using default would be better in this case.

Doesn't docs.rs use those? Otherwise https://docs.rs/bevy/latest/bevy/animation/struct.AnimationClip.html shouldn't be present as it's behind the animation feature

The animation feature is enabled by default, hence why it's documented there.

@GuillaumeGomez
Copy link
Contributor Author

I cleaned up the list of features by using default directly.

Hmm, maybe we should enable all features?

Why not. Just to be sure: do all features provide code too which would appear in the documentation? But if this is the best solution, there is an even simpler way for docs.rs to do that. :)

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I would rather enable all features or stick to default features: we don't have any development only flags and I'd really like to avoid keeping two lists.

@GuillaumeGomez
Copy link
Contributor Author

Makes things simpler for me. :)

@GuillaumeGomez
Copy link
Contributor Author

I updated it so that all features are enabled when building on docs.rs. Btw, you can find more information about it here: https://docs.rs/about/metadata

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Nice, that's much easier to maintain.

@mockersf
Copy link
Member

mockersf commented Jan 11, 2023

I still don't understand the point of this?

Bevy pretty much doesn't have any public items that are behind features not already enabled by default. What does it change for docs.rs which already uses the default set of features? It's mostly just going to increase build time on docs.rs.

@GuillaumeGomez
Copy link
Contributor Author

I honestly don't remember what I was missing in the documentation that forced me to render them locally. Should have precised it in my first comment, my bad. Looking at default features list, serialize isn't listed. Is it something end users can use?

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jan 11, 2023

Yes, it is. I've been bitten before by the lack of Serialize in the docs (and the fact that it's not a default feature).

We shouldn't change it to be one, just remarking on its use to end users.

@GuillaumeGomez
Copy link
Contributor Author

Then I guess it answers @mockersf's concern. :)

@mockersf
Copy link
Member

Then is it really a good idea to have things on docs.rs that users won't find when they try to use them? I think docs.rs can't yet automatically mark those things as "hey it's behind a feature that you need to enable"

We shouldn't change it to be one, just remarking on its use to end users.

Why? If it's useful it should be in the default feature set I think.

@GuillaumeGomez
Copy link
Contributor Author

It's easier to find that a feature is needed rather than finding that a feature exists if it's not in the documentation.

As a sidenote, we are currently writing an RFC for the "cfg_doc" feature, so wouldn't hurt to have it in any case.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Jan 11, 2023
@alice-i-cecile
Copy link
Member

Why? If it's useful it should be in the default feature set I think.

Serde leads to a large increase in compile times :( Maybe it should be on by default, but it's not a straightforward decision.

@GuillaumeGomez
Copy link
Contributor Author

A big poll to get people's opinion on this could help maybe? Personally I'm very happy serde isn't enabled by default as I don't use it. Don't know how common it is though.

@mockersf
Copy link
Member

mockersf commented Jan 11, 2023

serde is still enabled in the default features. the serialise feature wasn't done in all sub crates, and bevy_gltf has a mandatory dependency on serde anyway

@mockersf
Copy link
Member

done in #12366

@mockersf mockersf closed this May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-Docs An addition or correction to our documentation X-Controversial There is active debate or serious implications around merging this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants