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

Add documentation to note that some diagnostic plugins must be added after DiagnosticsPlugin #8785

Closed
wants to merge 5 commits into from
Closed

Add documentation to note that some diagnostic plugins must be added after DiagnosticsPlugin #8785

wants to merge 5 commits into from

Conversation

opstic
Copy link
Contributor

@opstic opstic commented Jun 8, 2023 •
edited
Loading

Objective

  • Some diagnostic plugins do not currently have docs to explain that it must be added after DiagnosticsPlugin

Solution

  • Add documentation for it

@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation A-Diagnostics Logging, crash handling, error reporting and performance analysis labels Jun 8, 2023
@alice-i-cecile
Copy link
Member

This should be reverted when #1255 is fixed.

alice-i-cecile
alice-i-cecile requested changes Jun 8, 2023
View reviewed changes
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.

Formatting / style nit + more clarity on why the panic occurs, then LGTM.

@opstic opstic requested a review from alice-i-cecile June 8, 2023 01:50
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 8, 2023
@mockersf
Copy link
Member

mockersf commented Jun 8, 2023

If we add this doc for FrameTimeDiagnosticsPlugin, it should be added to all the other diagnostics plugins:

  • AssetCountDiagnosticsPlugin
  • EntityCountDiagnosticsPlugin
  • SystemInformationDiagnosticsPlugin

@opstic opstic changed the title Add documentation to note that FrameTimeDiagnosticsPlugin must be added after DiagnosticsPlugin Add documentation to note that some diagnostic plugins must be added after DiagnosticsPlugin Jun 9, 2023
@opstic opstic requested a review from alice-i-cecile June 10, 2023 01:38
@mockersf mockersf mentioned this pull request Jun 10, 2023
Closed
@mockersf mockersf self-requested a review June 10, 2023 22:55
@mockersf
Copy link
Member

see #8783 (comment)

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

opstic commented Jun 11, 2023

Closing in favor of #8819

@opstic opstic closed this Jun 11, 2023
@opstic opstic deleted the add-diagnostics-docs branch June 13, 2023 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@mockersf mockersf Awaiting requested review from mockersf

Assignees
No one assigned
Labels
A-Diagnostics Logging, crash handling, error reporting and performance analysis C-Docs An addition or correction to our documentation S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@opstic @alice-i-cecile @mockersf