Add documentation for set_target_bitmap #25
Conversation
allegro/src/bitmap.rs
Outdated
| /// ``` | ||
| /// use allegro::*; | ||
| /// | ||
| /// pub fn blank_bitmap(core: &Core) { |
There was a problem hiding this comment.
"blank" implies something like black to me, but new bitmaps have undefined contents. I'm not sure this function is actually that useful. If you think this needs an example, then something that does: new bitmap + set target + clear to color would be more useful.
There was a problem hiding this comment.
"this function is actually that useful"
I usually draw to a blank bitmap then pass the pointer to that bitmap to a struct. When I'm ready to draw I set target to the backbuffer to and use al_draw_bitmap(*bmp, 0, 0, 0) then flip the display. It helps me keep track of things that way.
I'm also not sure these documentation submissions are desired which is why I wanted to test the waters for now by submitting a few pr's. If you don't think they are needed or useful in general let me know. My feelings won't be hurt
Just to clarify. My goal of with the examples is to create a minimum viable example (At least for now) since I'm a beginner. If you would rather someone with more experience with your library do the examples that's fine. I just thought I'd try to do something (Maybe) useful while I work through it.
I'll just keep learning the library for now either way. I'm pretty new to rust and just seeing a minimum example is useful to me. Especially coming from a allegro C user to using this wrapper.
I might make another push with the changes you suggest if you think this will be useful.
There was a problem hiding this comment.
I think it's okay if some functions don't have examples. I think my personal philosphy would be to have a quick example for the entire class in the class's comment, but for individual functions I'd only add them when it's appropriate.
There was a problem hiding this comment.
Understood. I'll consider that when submitting future PR's to documentation comments on a class level.
Feel free to close or cherry pick anything from this PR that you think would be helpful.
I "thought" this would have been helpful, but I'm new to Rust so having things spelled out to me is helpful.
I may come back later after spending more time with the language with some more insightful documentation.
There was a problem hiding this comment.
I think even this is pretty good, thank you. Tell me if you think you're done with it and I can merge it.
There was a problem hiding this comment.
That's good. I pushed a new commit to the branch I'm trying to merge in Battojutsu:jutsu/documentation
I appreciate the code review. It was insightful. I'll submit some more documentation updates later now that I see you are open to them. I'll be sure to try to keep them in separate commits so you can cherry-pick what you think is needed.
allegro/src/bitmap.rs
Outdated
| /// This method is used to load a bitmap file into a Bitmap object. | ||
| /// Takes a string path_to_map that points to a bitmap file. | ||
| /// # Examples | ||
| /// ``` |
There was a problem hiding this comment.
I am not able to see it the extra leading space myself..
There was a problem hiding this comment.
There's two spaces between /// and ```, one is extraneous.
Just testing the waters to see if any documentation updates on the functions would be desired or not. I had trouble figuring out how to use the core set_target_bitmap function and thought an example like this would have been helpful.