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

Document all public types #9

Merged
merged 10 commits into from
Nov 25, 2023
Merged

Conversation

Hellzbellz123
Copy link
Contributor

Kinda opinionated, I feel.
Also, a generic crate level README.

I'm a pretty big fan of these Clippy lints:
“missing-docs”,
// less useful for plugins/libraries
// not enabled for this PR
“clippy::missing_docs_in_private_items”

Not intended to be merged just yet, if at all. This is probably the one that's going to need the most refinement.

kinda opinionated
also a generic crate level readme.
@Hellzbellz123
Copy link
Contributor Author

Hellzbellz123 commented Nov 24, 2023

google is helpful :/

is there is a way to disable doc test for examples?
honestly i dont know if the example at the top is worth the effort.

also sorta feel the same way about documenting everything but it really does come down too "feel"

Copy link
Owner

@johanhelsing johanhelsing left a comment

Choose a reason for hiding this comment

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

“missing-docs”,

Yeah, agree this would be ideal.

src/behavior.rs Outdated Show resolved Hide resolved
src/behavior.rs Outdated Show resolved Hide resolved
src/gamepad.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/ui.rs Outdated
pub cursor_pos: RelativeCursorPosition,
}

/// bevy ui config for a stick
/// marker component containing `TouchStickId`
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not really sure what this component is for anymore. Can we remove it? In any case, "marker" components are generally unit structs (without fields).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can definitely be removed.
I only called it a marker component because of how it appeared to be used.

i did not want too add or remove types/fn with this commit, but if that is acceptable i can remove it

Copy link
Owner

Choose a reason for hiding this comment

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

Agree it's good that this is a pure doc pr

@johanhelsing
Copy link
Owner

is there is a way to disable doc test for examples? honestly i dont know if the example at the top is worth the effort.

There is

```ignore

But I don't think we should use it unless we absolutely have to. I think tested or at least compiled doc comments are great.

applies changes from code review so i can pull locally

Co-authored-by: Johan Klokkhammer Helsing <johanhelsing@gmail.com>
@Hellzbellz123 Hellzbellz123 changed the title all public types are documented Document all public types Nov 24, 2023
@Hellzbellz123
Copy link
Contributor Author

Hellzbellz123 commented Nov 24, 2023

does this need too be rebased on the most recent commits?
i noticed that the gamepad.rs on this branch does not have the updated TOUCH_GAMEPAD_ID and related comment?
will git solve this on its own when merged?

pulled from commit im about too push. purposely didnt include because main has other changes

docs for TOUCH_GAMEPAD_ID shouldnt include why its u32
if this does stay, it should be a normal comment.

// HACK: chosen at random, we're betting on no collisions with gilrs gamepads
// must be less than `u32::MAX` or breaks on 32bit platforms.
/// GamepadID for faked events
const TOUCH_GAMEPAD_ID: usize = 4264937294;

@Hellzbellz123
Copy link
Contributor Author

I think this is ready to merge.
I ran clippy::pedantic and noticed some code that didn't really match the rest of the style, so I “fixed” it.

Furthermore, I did not change the doc comment for TOUCH_GAMEPAD_ID, but I did back-tick the const being used

Copy link
Owner

@johanhelsing johanhelsing left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for all the PRs :)

src/ui.rs Outdated Show resolved Hide resolved
@johanhelsing
Copy link
Owner

docs for TOUCH_GAMEPAD_ID shouldnt include why its u32
if this does stay, it should be a normal comment.

Since this is internal API (not pub), I think it's good to have it as a doc comment.

@johanhelsing johanhelsing merged commit d56bc27 into johanhelsing:main Nov 25, 2023
3 checks passed
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