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

Fix prelude module docs inconsistency #9559

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Fix prelude module docs inconsistency #9559

wants to merge 8 commits into from
+19 −10

Conversation

tguichaoua
Copy link
Contributor

Objective

Solution

  • Fix prelude modules documentation inconsistency.

nicopap
nicopap reviewed Aug 24, 2023
View reviewed changes
crates/bevy_log/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_core/src/lib.rs Outdated Show resolved Hide resolved
@nicopap nicopap added C-Docs An addition or correction to our documentation C-Code-Quality A section of code that is hard to understand or change labels Aug 24, 2023
ickshonpe
ickshonpe approved these changes Aug 24, 2023
View reviewed changes
Copy link
Contributor

@ickshonpe ickshonpe left a comment •
edited
Loading

Choose a reason for hiding this comment

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

All good except for the capital "P"s that were already pointed out.

@tguichaoua
Copy link
Contributor Author

Why should 'P's be capitalized?
I'm not native English, is this a grammar rule?

@mockersf
Copy link
Member

I would prefer to see them all as #[allow(missing_docs)]. Those doc comments don't really bring anything.

@mockersf mockersf added the X-Controversial There is active debate or serious implications around merging this PR label Aug 24, 2023
@tguichaoua
Copy link
Contributor Author

I would prefer to see them all as #[allow(missing_docs)]. Those doc comments don't really bring anything.

I didn't think about using #[allow(missing_docs)] it sounds to be actually better.

rparrett
rparrett approved these changes Aug 30, 2023
View reviewed changes
Copy link
Contributor

@rparrett rparrett left a comment

Choose a reason for hiding this comment

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

Needs a fmt with Rust 1.72

@mockersf
Copy link
Member

Looks good to me. Do we want to agree on the guidelines (#9558) before applying them?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mockersf mockersf mockersf left review comments

@nicopap nicopap nicopap left review comments

@rparrett rparrett rparrett approved these changes

@atlv24 atlv24 atlv24 approved these changes

@ickshonpe ickshonpe ickshonpe approved these changes

Assignees
No one assigned
Labels
C-Code-Quality A section of code that is hard to understand or change 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tguichaoua @mockersf @rparrett @atlv24 @nicopap @ickshonpe