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.

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

Put ClientDiagnosticsPlugin under diagnostics feature #295

Merged
merged 6 commits into from
Jun 26, 2024

Conversation

Shatur
Copy link
Contributor

@Shatur Shatur commented Jun 24, 2024

And make it part of the RepliconPlugins group.

It makes it much more discoverable. Planning to put under features scene, parent_sync, client and server as well in next PRs. Like Bevy does.

Copy link

codecov bot commented Jun 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.96%. Comparing base (9dc6c2e) to head (fcb8403).
Report is 16 commits behind head on 0.14.0-rc.

Additional details and impacted files
@@              Coverage Diff              @@
##           0.14.0-rc     #295      +/-   ##
=============================================
+ Coverage      90.94%   90.96%   +0.02%     
=============================================
  Files             38       37       -1     
  Lines           2329     2202     -127     
=============================================
- Hits            2118     2003     -115     
+ Misses           211      199      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Shatur Shatur marked this pull request as ready for review June 24, 2024 22:24
@Shatur Shatur requested a review from UkoeHB June 24, 2024 22:24
Comment on lines +646 to +647
/// Statistic will be collected only if the resource is present.
/// The resource is not added by default.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Statistic will be collected only if the resource is present.
/// The resource is not added by default.
/// Statistic will be collected only if the resource is present.
/// The resource is not added by default. See [`ClientDiagnosticsPlugin`].

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 thought about it, but it's under a feature :(

If I link to it, cargo doc --open will emit a warning about missing link. I will need to enable the feature explicitly in CLI to generate docs without warnings.

Maybe you know a way to conditionally include docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added. I will just always run --all-features for cargo doc to avoid warnings. Discoverability is important.

Copy link
Collaborator

@UkoeHB UkoeHB Jun 25, 2024

Choose a reason for hiding this comment

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

Maybe you know a way to conditionally include docs?

You should be able to get all docs with this (and also get the nice feature-requirement message in docs):

# Cargo.toml
[package.metadata.docs.rs]
all-features = true
rustdoc-args = ["--cfg", "docsrs"]
#[cfg_attr(docsrs, doc(cfg(feature = "diagnostics")))]
pub mod diagnostics;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done!
I used doc_auto_cfg, like Bevy does:
bevyengine/bevy#12366
bevyengine/bevy#12642

@Shatur Shatur requested a review from UkoeHB June 25, 2024 16:40
Shatur added 3 commits June 25, 2024 20:43
This separation makes more sense to me and allows me to require a feature for running `diagnostics` test.
@Shatur Shatur merged commit 8ea7d21 into 0.14.0-rc Jun 26, 2024
6 checks passed
@Shatur Shatur deleted the diagnostics-feature branch June 26, 2024 07:27
Shatur added a commit that referenced this pull request Jul 4, 2024
* Update to 0.14.0-rc.2

* Temporary disable CI for bevy_replicon_renet

* Minor stylistic changes

Just a few spaces and an intermediate veriable to make the code look a
little bit nicer.

* Bump version to 0.27.0-rc.1

* Remove bevy_replicon_renet from workspace

* Bump version to 0.27.0-rc.2

* Undo changes in bevy_replicon_renet [skip ci]

* Rework events organization

- Swap `receive` system between ServerEventsPlugin and `ClientEventsPlugin` to properly separate what what runs on client or server.
- Move `ServerEventsPlugin::reset` logic inside `ClientEventsPlugin::reset` because this logic runs on client.
- Move `server::events::event_data` module to `core::event_registry::server_event`.
- Move `client::events::event_data` module to `core::event_registry::client_event`.

No functional changes, except `ServerEventsPlugin` and `ClientEventsPlugin` can be disabled on client-only and server-only apps respectively.

Closes #276.

* Do not divide values per seconds by the number of messages

* Update changelog [skip ci]

* Put `ClientDiagnosticsPlugin` under `diagnostics` feature (#295)

* Put `scene` module under `scene` feature (enabled by default)

* Put `parent_sync` module under `parent_sync` feature

* Client-server-features (#298)

* Use GitHub's warning about semver compatibility

https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#alerts

* Document features better

I took inspiration from Bevy.

* Update src/lib.rs

* Move `bevy_replicon_renet` to a dedicated repository

* Bump version to 0.27.0-rc.3

* Fix copy-paste

* Speedup removals caching

The necessary method was provided in 0.14.

* Remove mentions of the RC

Without it it will be quite hard to read the changelog (users will have to check  the changes from RC) or I will need to copy all changes from RC for the upcoming 0.27.0.

I also removed it from the compatibility table as suggested by @UkoeHB. I also don't think that they will be useful... Keeping them will make the table harder to read.
Another option would be to keep them, but put under a spoiler.

---------

Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants