Skip to content

Docs images helper #190

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Docs images helper #190

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f1415d2
initial trail of docs_image feature
Aug 11, 2020
fd042d8
added save_svg for docs image feature and small formatting
Aug 11, 2020
09609e1
changed to cfg target instead of feature for docs_image
Aug 12, 2020
1ea82af
Added save_png helper for generating images for doc tests
Aug 29, 2020
b477fce
Merge branch 'master' into docs_images_helper
JosephLing Aug 29, 2020
4005175
fixed save_svg to no longer include test data
JosephLing Aug 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ once_cell = "1.3"

cfg-if = "0.1"

turtle_docs_helper = { path = "turtle_docs_helper", optional = true }
Copy link
Owner

Choose a reason for hiding this comment

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

This should really be a dev-dependency, even if it is optional.

I wonder if the dependency cfg syntax would work here. Something like this:

[target.'cfg(docs_images)'.dev-dependencies]
turtle_docs_helper = { path = "turtle_docs_helper", optional = true }

Copy link
Author

Choose a reason for hiding this comment

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

I have tried this out in the latest commit but can't seem to get it too work as it can't seem to find the crate when I import it. (I added some additional info to the commit message as well)

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for the trouble! I pulled the code locally and was able to get it working. Here's what you need in your Cargo.toml file:

[dev-dependencies]
bitvec = "0.17"
turtle_docs_helper = { path = "turtle_docs_helper" }

Note that the dependency is not optional since dev-dependencies are apparently not allowed to be optional (that's fine). I unfortunately wasn't able to get target.'cfg(docs_images)'.dev-dependencies to work. It seems like it only works for selecting platforms. :(

To run this, I needed to use the command:

RUSTDOCFLAGS="--cfg docs_image" cargo test --features "test unstable" --doc

A couple of things to note about this:

  • using cargo test instead of cargo doc to make sure the tests are actually run (instead of just generating documentation)
  • using the test and unstable features to make sure none of the tests in the code are left out
  • using --doc to make sure only doc tests are run since regular unit tests don't generate images

I think the reason you ran into so many issues was because I pointed you to CONTRIBUTING.md which uses cargo doc. I don't think there is any way to use a dev-dependency from cargo doc. Sorry about that!

Hopefully this resolves the issues you were running into. Don't hesitate to let me know if you have any other questions! :)

Copy link
Author

Choose a reason for hiding this comment

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

Sweet, that was going to be my default choice if target.'cfg(docs_images)'.dev-dependencies couldn't work :) I think the main difficulty was the first time playing around with cfg to this extent.

Copy link
Author

Choose a reason for hiding this comment

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

After implementing the dev-dependancy chance I ran into another issue, the SaveSvg wasn't being implemented for the the Turtle struct. From testing out a fresh project I haven't been able to get the cfg flags to work for implementing traits for structs. As it won't run the implment block of code it seems. Here is the example I created:

// lib.rs for a new crate named "example"
pub struct Cats{}

#[cfg(docs_images)]
impl Cats{
    pub fn say(){
        println!("meow");
    }
}

///
/// ```rust
/// use example::Cats;
/// # #[cfg(docs_images)] Cats::say();
/// # #[cfg(docs_images)] assert_eq!(1,2);
/// 
/// ```
fn foo(){
    println!("hello world");
}

It should run the first test then fail the assert but the compiler can't find says(). (This is ran by using cargo test)

Potentailly it's the end of the week and I am missing something obivious as you said you were able to get it working.

Copy link
Owner

Choose a reason for hiding this comment

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

Could you tell me the command you used to run the program and paste the error message?

Copy link
Author

@JosephLing JosephLing Aug 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

running 1 test
test src\turtle.rs - turtle::Turtle::clear (line 817) ... FAILED

failures:

---- src\turtle.rs - turtle::Turtle::clear (line 817) stdout ----
error[E0277]: the trait bound `turtle::Turtle: turtle_docs_helper::SaveSvg` is not satisfied
  --> src\turtle.rs:824:58
   |
10 | #[cfg(docs_images)] turtle_docs_helper::save_docs_images(&turtle, "clear_before_click");
   |                                                          ^^^^^^^ the trait `turtle_docs_helper::SaveSvg` is not implemented for `turtle::Turtle`
   |
  ::: C:\Users\Joe\workspace\turtle\turtle_docs_helper\src\lib.rs:12:28
   |
12 | pub fn save_docs_images<T: SaveSvg>(drawing: &T, output_name: &str) {
   |                            ------- required by this bound in `turtle_docs_helper::save_docs_images`

error[E0277]: the trait bound `turtle::Turtle: turtle_docs_helper::SaveSvg` is not satisfied
  --> src\turtle.rs:827:58
   |
13 | #[cfg(docs_images)] turtle_docs_helper::save_docs_images(&turtle, "clear_after_click");
   |                                                          ^^^^^^^ the trait `turtle_docs_helper::SaveSvg` is not implemented for `turtle::Turtle`
   |
  ::: C:\Users\Joe\workspace\turtle\turtle_docs_helper\src\lib.rs:12:28
   |
12 | pub fn save_docs_images<T: SaveSvg>(drawing: &T, output_name: &str) {
   |                            ------- required by this bound in `turtle_docs_helper::save_docs_images`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0277`.
Couldn't compile the test.

failures:
    src\turtle.rs - turtle::Turtle::clear (line 817)

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 146 filtered out

error: test failed, to rerun pass '--doc'
PS C:\Users\Joe\workspace\turtle> cargo test --features "test unstable" --doc --package turtle -- turtle::Turtle::clear

with RUSTDOCFLAGS set to --cfg docs_images in the env

I can push the code as well although there haven't really be any major changes.

EDIT: perhaps as I am running as a docs flag it is only affecting the doctests and nothing else 🤦‍♂️

Copy link
Owner

Choose a reason for hiding this comment

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

perhaps as I am running as a docs flag it is only affecting the doctests and nothing else

That is possible! Totally not your fault. I would personally expect it to work the way we have it. You can try fixing it by setting RUSTFLAGS in addition to RUSTDOCFLAGS. Let me know if it works!

And thanks for taking the time to experiment!

Copy link
Author

Choose a reason for hiding this comment

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

Yep because doc tests run kind of like integeration tests that was the problem. As the code it was running on didn't get the cfg flag passed to it. For turtle_docs_helper to work as a crate we need it to only be required when running tests so:

#[cfg(all(tests, docs_images))]
use turtle_docs_helper;

As its a dev dependency so not found when running the main build (cargo build ). So that should fix everything but we will see if anything else explodes when I next work on it haha

Copy link
Author

Choose a reason for hiding this comment

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

Basically I think a feature flag like orginal commit is the only way to get this work and the nicest way I think. Writing the code is a bit more verbose but it's really simple to run and actually runs.

One of the issues is in order for cfg flags to work they are seperate for RUSTFLAGS and RUSTDOCFLAGS so that means you need both set if you want the doctests to run the cfg(docs_images).

I think because of that I didn't notice that this:


#[cfg(all(test, docs_images))]
use turtle_docs_helper;


pub struct Cats{

}

#[cfg(all(test, docs_images))]
impl Cats{
    pub fn say(){
        println!("meow");
        turtle_docs_helper::cats();
    }
}

///
/// ```rust
/// use example::Cats;
/// # #[cfg(docs_images)] Cats::say();
/// # #[cfg(docs_images)] assert_eq!(turtle_docs_helper::cats(), 4);
/// # #[cfg(docs_images)] assert_eq!(1,2);
/// # #[cfg(docs_images)] assert_eq!(3,5);
/// assert_eq!(5,2);
/// 
/// ```
fn foo(){
    println!("hello world");
}

Doesn't work as this won't run (for test or tests):

#[cfg(all(test, docs_images))]
impl Cats{
    pub fn say(){
        println!("meow");
        turtle_docs_helper::cats();
    }
}

The use turtle_docs_helper will run so that module does get imported however you get the error:

---- src\lib.rs - foo (line 18) stdout ----
error[E0599]: no function or associated item named `say` found for struct `example::Cats` in the current scope
 --> src\lib.rs:20:27
  |
5 | #[cfg(docs_images)] Cats::say();
  |                           ^^^ function or associated item not found in `example::Cats`

error: aborting due to previous error


[dependencies.futures-util]
version = "0.3"
default-features = false
Expand Down Expand Up @@ -90,7 +92,8 @@ opt-level = 3

[features]
# The reason we do this is because doctests don't get cfg(test)
# See: https://github.com/rust-lang/cargo/issues/4669
# See: https://github.com/rust-lang/cargo/issues/4669 (original issue)
# See: https://github.com/rust-lang/rust/issues/45599 (updated issue)
#
# This allows us to write attributes like the following and have it work
# in all tests.
Expand All @@ -105,3 +108,4 @@ test = []
#
# Users of the crate must explicitly opt-in to activate them.
unstable = []
docs_image = ["turtle_docs_helper"]
Copy link
Owner

Choose a reason for hiding this comment

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

I think I would still prefer to have this be a special --cfg flag rather than an actual feature in the Cargo.toml file. After all, no one is meant to use this, it's only to help us automate generating the images. Did you run into any issues trying to set that up?

Also, a small nit: let's name this docs_images instead of docs_image.

21 changes: 15 additions & 6 deletions src/async_drawing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
use std::fmt::Debug;
use std::path::Path;

use serde::{Serialize, Deserialize};
use serde::{Deserialize, Serialize};

use crate::ipc_protocol::ProtocolClient;
use crate::async_turtle::AsyncTurtle;
use crate::{Drawing, Point, Color, Event, ExportError};
use crate::ipc_protocol::ProtocolClient;
use crate::{Color, Drawing, Event, ExportError, Point};

/// Represents a size
///
Expand Down Expand Up @@ -64,16 +64,25 @@ impl From<Drawing> for AsyncDrawing {
}
}

use crate::sync_runtime::block_on;
#[cfg(feature = "docs_image")]
use turtle_docs_helper;
#[cfg(feature = "docs_image")]
impl turtle_docs_helper::SaveSvg for AsyncDrawing {
fn save_svg(&self, path: &Path) -> Result<(), String> {
self.client.save_svg(path)
}
}

impl AsyncDrawing {
pub async fn new() -> Self {
// This needs to be called as close to the start of the program as possible. We call it
// here since Drawing::new() or AsyncDrawing::new() are commonly called at the beginning
// of many programs that use the turtle crate.
crate::start();

let client = ProtocolClient::new().await
.expect("unable to create renderer client");
Self {client}
let client = ProtocolClient::new().await.expect("unable to create renderer client");
Self { client }
}

pub async fn add_turtle(&mut self) -> AsyncTurtle {
Expand Down
50 changes: 37 additions & 13 deletions src/async_turtle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,19 @@ use std::fmt::Debug;

use tokio::time;

use crate::radians::{self, Radians};
use crate::ipc_protocol::{ProtocolClient, RotationDirection};
use crate::radians::{self, Radians};
use crate::renderer_server::TurtleId;
use crate::{Turtle, Color, Point, Speed};
use crate::{Color, Point, Speed, Turtle};

#[cfg(feature = "docs_image")]
use turtle_docs_helper;

#[cfg(feature = "docs_image")]
use std::path::Path;

#[cfg(feature = "docs_image")]
use crate::sync_runtime::block_on;

/// Any distance value (positive or negative)
pub type Distance = f64;
Expand Down Expand Up @@ -45,6 +54,16 @@ pub struct AsyncTurtle {
angle_unit: AngleUnit,
}

#[cfg(feature = "docs_image")]
impl turtle_docs_helper::SaveSvg for AsyncTurtle {
fn save_svg(&self, path: &Path) -> Result<(), String> {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
fn save_svg(&self, path: &Path) -> Result<(), String> {
fn save_svg(&self, path: &std::path::Path) -> Result<(), String> {

We can avoid having to import Path by using its fully qualified name here. Please do this for the other impls as well.

match self.client.save_svg(path) {
Ok(()) => Ok(()),
Err(e) => Err(e.to_string()),
}
}
}

impl From<Turtle> for AsyncTurtle {
fn from(turtle: Turtle) -> Self {
turtle.into_async()
Expand All @@ -58,8 +77,7 @@ impl AsyncTurtle {
// of many programs that use the turtle crate.
crate::start();

let client = ProtocolClient::new().await
.expect("unable to create renderer client");
let client = ProtocolClient::new().await.expect("unable to create renderer client");
Self::with_client(client).await
}

Expand All @@ -68,7 +86,7 @@ impl AsyncTurtle {
let id = client.create_turtle().await;
let angle_unit = AngleUnit::Degrees;

Self {client, id, angle_unit}
Self { client, id, angle_unit }
}

pub async fn forward(&mut self, distance: Distance) {
Expand All @@ -87,7 +105,9 @@ impl AsyncTurtle {

pub async fn left(&mut self, angle: Angle) {
let angle = self.angle_unit.to_radians(angle);
self.client.rotate_in_place(self.id, angle, RotationDirection::Counterclockwise).await
self.client
.rotate_in_place(self.id, angle, RotationDirection::Counterclockwise)
.await
}

pub async fn wait(&mut self, secs: f64) {
Expand Down Expand Up @@ -122,13 +142,13 @@ impl AsyncTurtle {
}

pub async fn set_x(&mut self, x: f64) {
let Point {x: _, y} = self.position().await;
self.go_to(Point {x, y}).await
let Point { x: _, y } = self.position().await;
self.go_to(Point { x, y }).await
}

pub async fn set_y(&mut self, y: f64) {
let Point {x, y: _} = self.position().await;
self.go_to(Point {x, y}).await
let Point { x, y: _ } = self.position().await;
self.go_to(Point { x, y }).await
}

pub async fn home(&mut self) {
Expand All @@ -155,7 +175,9 @@ impl AsyncTurtle {
// Formula from: https://stackoverflow.com/a/24234924/551904
let angle = angle - radians::TWO_PI * ((angle + radians::PI) / radians::TWO_PI).floor();

self.client.rotate_in_place(self.id, angle, RotationDirection::Counterclockwise).await
self.client
.rotate_in_place(self.id, angle, RotationDirection::Counterclockwise)
.await
}

pub fn is_using_degrees(&self) -> bool {
Expand Down Expand Up @@ -291,13 +313,15 @@ impl AsyncTurtle {
angle
};

self.client.rotate_in_place(self.id, angle, RotationDirection::Counterclockwise).await
self.client
.rotate_in_place(self.id, angle, RotationDirection::Counterclockwise)
.await
}

pub async fn wait_for_click(&mut self) {
use crate::{
event::{MouseButton::LeftButton, PressedState::Pressed},
Event::MouseButton,
event::{PressedState::Pressed, MouseButton::LeftButton},
};

loop {
Expand Down
86 changes: 75 additions & 11 deletions src/drawing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
use std::fmt::Debug;
use std::path::Path;

use crate::{Turtle, Color, Point, Size, ExportError};
use crate::async_drawing::AsyncDrawing;
use crate::sync_runtime::block_on;
use crate::{Color, ExportError, Point, Size, Turtle};

#[cfg(feature = "docs_image")]
use turtle_docs_helper;

/// Provides access to properties of the drawing that the turtle is creating
///
Expand Down Expand Up @@ -63,7 +66,17 @@ impl From<AsyncDrawing> for Drawing {
fn from(drawing: AsyncDrawing) -> Self {
//TODO: There is no way to set `turtles` properly here, but that's okay since it is going
// to be removed soon.
Self {drawing, turtles: 1}
Self { drawing, turtles: 1 }
}
}

#[cfg(feature = "docs_image")]
impl turtle_docs_helper::SaveSvg for Drawing {
fn save_svg(&self, path: &Path) -> Result<(), String> {
match block_on(self.drawing.save_svg(path)) {
Ok(()) => Ok(()),
Err(e) => Err(e.to_string()),
}
}
}

Expand Down Expand Up @@ -142,14 +155,17 @@ impl Drawing {
///
/// # Example
///
/// ```rust,no_run
/// ```rust
/// use turtle::Drawing;
///
/// fn main() {
/// let mut drawing = Drawing::new();
/// # #[allow(unused)] // Good to show turtle creation here even if unused
/// let mut turtle = drawing.add_turtle();
/// drawing.set_title("My Fancy Title! - Yay!");
/// # #[cfg(feature = "docs_image")]
/// # turtle_docs_helper::save_docs_image(&drawing, "changed_title");
///
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
/// # #[cfg(feature = "docs_image")]
/// # turtle_docs_helper::save_docs_image(&drawing, "changed_title");
///
/// # #[cfg(feature = "docs_image")] turtle_docs_helper::save_docs_image(&drawing, "changed_title");

Let's put the #[cfg] on the same line as the function call in all places where we call save_docs_image. None of this code is "for display" since the leading # hides it all anyway, so I would prefer that we keep it as terse as possible. :)

/// }
/// ```
///
Expand Down Expand Up @@ -181,14 +197,16 @@ impl Drawing {
///
/// # Example
///
/// ```rust,no_run
/// ```rust
/// use turtle::Drawing;
///
/// fn main() {
/// let mut drawing = Drawing::new();
/// # #[allow(unused)] // Good to show turtle creation here even if unused
/// let mut turtle = drawing.add_turtle();
/// drawing.set_background_color("orange");
/// # #[cfg(feature = "docs_image")]
/// # turtle_docs_helper::save_docs_image(&drawing, "orange_background");
/// }
/// ```
///
Expand Down Expand Up @@ -223,7 +241,7 @@ impl Drawing {
///
/// # Example
///
/// ```rust,no_run
/// ```rust
/// use turtle::Drawing;
///
/// fn main() {
Expand All @@ -236,9 +254,31 @@ impl Drawing {
/// // Rotate to the right (clockwise) by 1 degree
/// turtle.right(1.0);
/// }
///
/// # #[cfg(feature = "docs_image")]
/// # turtle_docs_helper::save_docs_image(&drawing, "circle");
/// # }
/// ```
/// ```rust, no_run
/// # use turtle::Drawing;
/// # let mut drawing = Drawing::new();
/// # let mut turtle = drawing.add_turtle();
/// turtle.wait_for_click();
/// ```
/// ```rust
/// # use turtle::Drawing;
/// # fn main() {
/// # let mut drawing = Drawing::new();
/// # let mut turtle = drawing.add_turtle();
///
/// # for _ in 0..360 {
/// # // Move forward three steps
/// # turtle.forward(3.0);
/// # // Rotate to the right (clockwise) by 1 degree
/// # turtle.right(1.0);
/// # }
/// drawing.set_center([50.0, 100.0]);
/// # #[cfg(feature = "docs_image")]
/// # turtle_docs_helper::save_docs_image(&drawing, "circle_offset_center");
/// }
/// ```
///
Expand Down Expand Up @@ -306,7 +346,7 @@ impl Drawing {
///
/// # Example
///
/// ```rust,no_run
/// ```rust
/// use turtle::Drawing;
///
/// fn main() {
Expand All @@ -319,9 +359,31 @@ impl Drawing {
/// // Rotate to the right (clockwise) by 1 degree
/// turtle.right(1.0);
/// }
///
/// # #[cfg(feature = "docs_image")]
/// # turtle_docs_helper::save_docs_image(&drawing, "circle");
/// # }
/// ```
/// ```rust, no_run
/// # use turtle::Drawing;
/// # let mut drawing = Drawing::new();
/// # let mut turtle = drawing.add_turtle();
/// turtle.wait_for_click();
/// ```
/// ```rust
/// # use turtle::Drawing;
/// # fn main() {
/// # let mut drawing = Drawing::new();
/// # let mut turtle = drawing.add_turtle();
///
/// # for _ in 0..360 {
/// # // Move forward three steps
/// # turtle.forward(3.0);
/// # // Rotate to the right (clockwise) by 1 degree
/// # turtle.right(1.0);
/// # }
/// drawing.set_size((300, 300));
/// # #[cfg(feature = "docs_image")]
/// # turtle_docs_helper::save_docs_image(&drawing, "small_drawing");
/// }
/// ```
///
Expand Down Expand Up @@ -570,7 +632,7 @@ impl Drawing {

/// Saves the current drawings in SVG format at the location specified by `path`.
///
/// ```rust,no_run
/// ```rust
/// use turtle::{Drawing, Turtle, Color, ExportError};
///
/// fn main() -> Result<(), ExportError> {
Expand Down Expand Up @@ -619,7 +681,9 @@ mod tests {
use super::*;

#[test]
#[should_panic(expected = "Invalid color: Color { red: NaN, green: 0.0, blue: 0.0, alpha: 0.0 }. See the color module documentation for more information.")]
#[should_panic(
expected = "Invalid color: Color { red: NaN, green: 0.0, blue: 0.0, alpha: 0.0 }. See the color module documentation for more information."
)]
fn rejects_invalid_background_color() {
let mut drawing = Drawing::new();
drawing.set_background_color(Color {
Expand All @@ -640,7 +704,7 @@ mod tests {

#[test]
fn ignores_center_nan_inf() {
let center = Point {x: 5.0, y: 10.0};
let center = Point { x: 5.0, y: 10.0 };

let mut drawing = Drawing::new();
drawing.set_center(center);
Expand Down
Loading