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

[Merged by Bors] - Add documentation to the WindowDescriptor struct. #4764

Closed
wants to merge 12 commits into from
Closed

[Merged by Bors] - Add documentation to the WindowDescriptor struct. #4764

wants to merge 12 commits into from

Conversation

object71
Copy link
Contributor

Objective

Resolves #4753

Solution

Using rust doc I added documentation to the struct. Decided to not provide an example in the doc comment but instead refer to the example file that shows the usage.

@object71 object71 force-pushed the document-window-descriptor branch 2 times, most recently from 01342e4 to f7e494a Compare May 16, 2022 14:32
@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation A-Windowing Platform-agnostic interface layer to run your app in labels May 16, 2022
alice-i-cecile
alice-i-cecile reviewed May 16, 2022
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.

These are excellent docs. Some small nits, but this is nearly ready.

@object71 object71 force-pushed the document-window-descriptor branch from cb5e626 to 1f10578 Compare May 17, 2022 05:14
@alice-i-cecile alice-i-cecile self-requested a review May 17, 2022 05:21
@object71 object71 force-pushed the document-window-descriptor branch from 1f10578 to afdc97f Compare May 17, 2022 10:44
@alice-i-cecile
Copy link
Member

@bevyengine/docs-team can I get a second review?

ghost
ghost approved these changes May 17, 2022
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Great work! Especially the comment on the struct itself will be really beneficial for a lot of users.

@object71 object71 force-pushed the document-window-descriptor branch from afdc97f to a1e1770 Compare May 17, 2022 18:37
@object71
Copy link
Contributor Author

Also added a short comment for decorations in the last change as it isn't descriptive at first glance (or at least for me). So I searched it in winit and found that it is responsible for borders and bars.

@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 May 17, 2022
@alice-i-cecile
Copy link
Member

Awesome, thanks again! BTW, in the future, prefer many small commits over rebasing when contributing here. We squash on merge, and the separate commits make it easier for reviewers to quickly check what's changed.

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request May 17, 2022
@object71
Add documentation to the WindowDescriptor struct. (#4764)
b72fbfe 
# Objective

Resolves #4753

## Solution

Using rust doc I added documentation to the struct. Decided to not provide an example in the doc comment but instead refer to the example file that shows the usage.
@bors
Copy link
Contributor

bors bot commented May 17, 2022

Build failed:

@alice-i-cecile
Copy link
Member

[✖] http://blog.izs.me/post/30036893703/policy-on-trolling → Status: 0 Error: ETIMEDOUT
at Timeout. (/usr/local/lib/node_modules/markdown-link-check/node_modules/request/request.js:848:19)
at listOnTimeout (node:internal/timers:564:17)
at process.processTimers (node:internal/timers:507:7) {
code: 'ETIMEDOUT',
connect: true
}
[✖] https://www.contributor-covenant.org/ → Status: 0 Error: ETIMEDOUT
at Timeout. (/usr/local/lib/node_modules/markdown-link-check/node_modules/request/request.js:848:19)
at listOnTimeout (node:internal/timers:564:17)
at process.processTimers (node:internal/timers:507:7) {
code: 'ETIMEDOUT',
connect: true
}

Ah, #4575 strikes again. Not your fault here.

@object71
Copy link
Contributor Author

y check what's changed.

Ok, thanks for the guideline :)

Nilirad
Nilirad suggested changes May 18, 2022
View reviewed changes
Copy link
Contributor

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

This is a good improvement. Adding docs is always helpful to readers who want to learn more.

To my eyes this PR only needs some adjustments on what has been already changed. I specified them in the comments below.

Also, I feel that this needs some other additions, in particular to undocumented fields, because #4753 requires that all fields should be documented to be closed.

  • title,
  • resizable,
  • cursor_visible,
  • cursor_locked,

have been probably left out because they are trivial, but a well thought short description is always helpful.

The fields:

  • resize_constraints
  • present_mode
  • mode

are even easier to document, since a link to the type with a minimal description is sufficient.

crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
Nilirad
Nilirad suggested changes May 19, 2022
View reviewed changes
Copy link
Contributor

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

Great work with documenting all fields!

Once the wording suggestion below is applied, I will feel comfortable to approve this PR. 😄

crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
@object71
Copy link
Contributor Author

I don't understand why it fails. Do I need to do something else?

@alice-i-cecile
Copy link
Member

Not your fault; this was a flaky check that we've since removed.

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request May 30, 2022
@object71
Add documentation to the WindowDescriptor struct. (#4764)
6a23837 
# Objective

Resolves #4753

## Solution

Using rust doc I added documentation to the struct. Decided to not provide an example in the doc comment but instead refer to the example file that shows the usage.
@bors
Copy link
Contributor

bors bot commented May 30, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Add documentation to the WindowDescriptor struct. [Merged by Bors] - Add documentation to the WindowDescriptor struct. May 30, 2022
@bors bors bot closed this May 30, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 7, 2022
@object71 @james7132
Add documentation to the WindowDescriptor struct. (bevyengine#4764)
9f4caca 
# Objective

Resolves bevyengine#4753

## Solution

Using rust doc I added documentation to the struct. Decided to not provide an example in the doc comment but instead refer to the example file that shows the usage.
@object71 object71 deleted the document-window-descriptor branch September 14, 2022 09:20
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@object71 @ItsDoot
Add documentation to the WindowDescriptor struct. (bevyengine#4764)
2d94217 
# Objective

Resolves bevyengine#4753

## Solution

Using rust doc I added documentation to the struct. Decided to not provide an example in the doc comment but instead refer to the example file that shows the usage.
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

@IceSentry IceSentry IceSentry approved these changes

@Nilirad Nilirad Nilirad approved these changes

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

Assignees
No one assigned
Labels
A-Windowing Platform-agnostic interface layer to run your app in 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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add docs to WindowDescriptor
5 participants
@object71 @alice-i-cecile @IceSentry @mockersf @Nilirad