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

bevy_reflect: Improve List::push and List::pop docs #6866

Closed
MrGVSV opened this issue Dec 6, 2022 · 4 comments
Closed

bevy_reflect: Improve List::push and List::pop docs #6866

MrGVSV opened this issue Dec 6, 2022 · 4 comments
Labels
A-Reflection Runtime information about types C-Docs An addition or correction to our documentation D-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@MrGVSV
Copy link
Member

MrGVSV commented Dec 6, 2022

How can Bevy's documentation be improved?

The docs on List::push and List::pop should better explain the directionality of those operations, as explained in this comment.

We should direct manual (and internal) implementors to uphold the contract that:

  1. "pushing" always appends the element to the end of the list
  2. "popping" always removes the element from the end of the list

The docs already do this to an extent, but they could give just a bit more detail and make the guarantee more explicit so implementors don't miss it!

@MrGVSV MrGVSV added C-Docs An addition or correction to our documentation A-Reflection Runtime information about types D-Trivial Nice and easy! A great choice to get started with Bevy labels Dec 6, 2022
@LinusKall
Copy link
Contributor

LinusKall commented Dec 6, 2022

I clarified the meaning of first/front element and last/back element with some emphasis. What do you think, should I make a PR? Suggestions?

/// An ordered, mutable list of [Reflect] items. This corresponds to types like [`std::vec::Vec`].
///
/// This is a sub-trait of [`Array`] as it implements a [`push`](List::push) function, allowing
/// it's internal size to grow.
///
/// This trait expects index 0 to contain the _first_ or _front_ element.
/// The _last_ or _back_ element refers to the element with the largest index.
pub trait List: Reflect + Array {
    /// Appends an element to the _back_ of the list.
    fn push(&mut self, value: Box<dyn Reflect>);

    /// Removes the _last_ element from the list and returns it, or [`None`] if it is empty.
    fn pop(&mut self) -> Option<Box<dyn Reflect>>;

    /// Clones the list, producing a [`DynamicList`].
    fn clone_dynamic(&self) -> DynamicList {
        DynamicList {
            name: self.type_name().to_string(),
            values: self.iter().map(|value| value.clone_value()).collect(),
        }
    }
}

@MrGVSV
Copy link
Member Author

MrGVSV commented Dec 6, 2022

Yeah I think that's pretty good. I think I like your choice in extracting that info into the docs for List itself rather than repeating it for each method. I wonder if words like last should backlink to those docs? 🤔

My only other comments are:

  • Personally I prefer end over back, but maybe that's just me
  • I’d include an additional sentence or two that explicitly call out manual implementors to remind them of these "rules"

If you can, I’d definitely make a PR so we can get more feedback from the community and iterate based off that!

@LinusKall
Copy link
Contributor

I wonder if words like last should backlink to those docs? 🤔

By the way, how do I create backlinks? 🤔

@tim-blackbird
Copy link
Contributor

Personally I prefer end over back, but maybe that's just me

Don't mind personally, but front/back would match the method names and docs for VecDeque so I think we should go with that.

@bors bors bot closed this as completed in e087013 Dec 7, 2022
alradish pushed a commit to alradish/bevy that referenced this issue Jan 22, 2023
# Objective

Fixes bevyengine#6866.

## Solution

Docs now should describe what the _front_, _first_, _back_, and _last_ elements are for an implementor of the `bevy::reflect::list::List` Trait. Further, the docs should describe how `bevy::reflect::list::List::push` and `bevy::reflect::list::List::pop` should act on these elements.


Co-authored-by: Linus Käll <linus.kall.business@gmail.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
# Objective

Fixes bevyengine#6866.

## Solution

Docs now should describe what the _front_, _first_, _back_, and _last_ elements are for an implementor of the `bevy::reflect::list::List` Trait. Further, the docs should describe how `bevy::reflect::list::List::push` and `bevy::reflect::list::List::pop` should act on these elements.


Co-authored-by: Linus Käll <linus.kall.business@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Docs An addition or correction to our documentation D-Trivial Nice and easy! A great choice to get started with Bevy
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants