Docs images helper #190
Docs images helper #190
Conversation
Currently using: cargo test --features "test docs_image" --doc --package turtle -- drawing::Drawing::set_title for testing however it isn't currently failing when it should do. As there is an assert_eq that should fail... and no files are being written to the path.
There was a problem hiding this comment.
Thanks for your work on this @JosephLing! You've done a great job!
I've left you a bunch of comments, mostly to help you clean things up and simplify some of the changes you've made. Once you get the images from svg -> png, please commit them so I can see the results. I'm hoping that the process will end up being just one command that puts all the SVGs and PNGs in the correct place.
check if this is correct approach
You are on the right track so far! I would really prefer to use cfg(docs_images) rather than a cargo feature. Please let me know if you run into any issues with that. Some of my comments provide links to documentation that may help you out. :)
improve svg rendering as the turtle currently isn't shown in the render which is needed for one or two examples
This is a hard problem. I could add something to the SVG exporter to draw the turtle, or we could change the examples to not require having the turtle in the image. I think we should go with a third option: skip automating those examples for now, and open an issue to figure out how to do that after this PR. We definitely don't need to try and do all of this work at once!
for the title bar I am not sure the best way to handle it but it is easy to get the data out.
Let's leave the examples that require the titlebar out of the automation. I have some ideas for that that we can work on as a follow up once this PR is merged. :)
Please don't hesitate to let me know if you have any questions! You can also ask on Zulip if you prefer that over GitHub comments. I am here to help. :)
Cargo.toml
Outdated
| @@ -105,3 +108,4 @@ test = [] | |||
| # | |||
| # Users of the crate must explicitly opt-in to activate them. | |||
| unstable = [] | |||
| docs_image = ["turtle_docs_helper"] | |||
There was a problem hiding this comment.
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.
Cargo.toml
Outdated
| @@ -46,6 +46,8 @@ once_cell = "1.3" | |||
|
|
|||
| cfg-if = "0.1" | |||
|
|
|||
| turtle_docs_helper = { path = "turtle_docs_helper", optional = true } | |||
There was a problem hiding this comment.
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 }There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 testinstead ofcargo docto make sure the tests are actually run (instead of just generating documentation) - using the
testandunstablefeatures to make sure none of the tests in the code are left out - using
--docto 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! :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Could you tell me the command you used to run the program and paste the error message?
There was a problem hiding this comment.
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 🤦♂️
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
src/ipc_protocol/protocol.rs
Outdated
| #[cfg(feature = "docs_image")] | ||
| impl turtle_docs_helper::SaveSvg for ProtocolClient { | ||
| fn save_svg(&self, path: &Path) -> Result<(), String> { | ||
| match block_on(self.export_svg(path.to_path_buf())) { |
There was a problem hiding this comment.
You can actually put the use crate::sync_runtime::block_on; line just above this match line to avoid having to have a separate #[cfg] just for that import.
src/drawing.rs
Outdated
| /// # #[cfg(feature = "docs_image")] | ||
| /// # turtle_docs_helper::save_docs_image(&drawing, "changed_title"); | ||
| /// |
There was a problem hiding this comment.
| /// # #[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. :)
I have cfg set to docs_image (I know I haven't renamed it yet to docs_images...) so that it runs all the cfg(docs_image) code. However it fails to import 'turtle_docs_helper' as it is not an external crate. From reading up on this it should work... although it does suggest just to use features for doing this kind of thing in the docs. https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#platform-specific-dependencies
|
@JosephLing Sorry about the late reply. I really wanted to make sure I get you unstuck, so I checked out the code locally and did some digging starting on the master branch. The patch below works and does everything up to the actual SVG image generation itself. You should just have to finish off the SVG to PNG conversion, test it out, add a call to
Having to set both A Better Starting PointA lot of the issues you ran into were because of my own incomplete understanding of some of Rust's features and cargo. Sorry about that! I've taken the time to fix that and I'm hoping that the patch below will be a much better starting point for your work on this. The first detail I missed was that dev-dependencies are only available for the actual test files themselves, the crate itself is not recompiled with the dev-dependencies. That means the only way to get this to work is to list Solution: have a commented out line in You can run the code with: # Don't forget to uncomment the `turtle_docs_helpers` dependency in Cargo.toml
RUSTDOCFLAGS="--cfg docs_images" RUSTFLAGS="--cfg docs_images" cargo test --features "test unstable" --docOn windows, you may need to set the environment variables at the front of the command in advance, then run the rest of the command starting with diff --git a/Cargo.toml b/Cargo.toml
index 1dcfbca..b103548 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -51,6 +51,11 @@ cfg-if = "0.1"
parking_lot = "0.10"
+# This dependency should be uncommented when rebuilding the images in the API
+# documentation. In all other cases this dependency cannot be used since having
+# a path-only dependency would make it impossible to publish this crate.
+#turtle_docs_helpers = { path = "turtle_docs_helpers" }
+
[dependencies.futures-util]
version = "0.3"
default-features = false
diff --git a/src/async_drawing.rs b/src/async_drawing.rs
index 42931ea..49176fe 100644
--- a/src/async_drawing.rs
+++ b/src/async_drawing.rs
@@ -188,3 +188,10 @@ impl AsyncDrawing {
self.client.debug_drawing().await
}
}
+
+#[cfg(docs_images)]
+impl turtle_docs_helpers::SaveDocsSvg for AsyncDrawing {
+ fn save_docs_svg(&self, path: std::path::PathBuf) {
+ self.client.save_docs_svg(path);
+ }
+}
diff --git a/src/async_turtle.rs b/src/async_turtle.rs
index 2602292..77f1e74 100644
--- a/src/async_turtle.rs
+++ b/src/async_turtle.rs
@@ -324,3 +324,10 @@ impl AsyncTurtle {
self.client.debug_turtle(self.id, self.angle_unit).await
}
}
+
+#[cfg(docs_images)]
+impl turtle_docs_helpers::SaveDocsSvg for AsyncTurtle {
+ fn save_docs_svg(&self, path: std::path::PathBuf) {
+ self.client.save_docs_svg(path);
+ }
+}
diff --git a/src/drawing.rs b/src/drawing.rs
index 014e9eb..6548edf 100644
--- a/src/drawing.rs
+++ b/src/drawing.rs
@@ -629,6 +629,13 @@ impl Drawing {
}
}
+#[cfg(docs_images)]
+impl turtle_docs_helpers::SaveDocsSvg for Drawing {
+ fn save_docs_svg(&self, path: std::path::PathBuf) {
+ self.drawing.save_docs_svg(path);
+ }
+}
+
#[cfg(test)]
mod tests {
use super::*;
diff --git a/src/ipc_protocol/protocol.rs b/src/ipc_protocol/protocol.rs
index 6e13beb..df9a9b3 100644
--- a/src/ipc_protocol/protocol.rs
+++ b/src/ipc_protocol/protocol.rs
@@ -408,3 +408,12 @@ impl ProtocolClient {
}
}
}
+
+#[cfg(docs_images)]
+impl turtle_docs_helpers::SaveDocsSvg for ProtocolClient {
+ fn save_docs_svg(&self, path: std::path::PathBuf) {
+ use crate::sync_runtime::block_on;
+ block_on(self.export_svg(path))
+ .expect("unable to save docs image");
+ }
+}
diff --git a/src/turtle.rs b/src/turtle.rs
index 5019456..a1e1388 100644
--- a/src/turtle.rs
+++ b/src/turtle.rs
@@ -535,7 +535,7 @@ impl Turtle {
///
/// # Example
///
- /// ```rust,no_run
+ /// ```rust
/// use turtle::Turtle;
///
/// fn main() {
@@ -557,6 +557,7 @@ impl Turtle {
/// turtle.set_pen_color("#4CAF50"); // green
/// turtle.set_pen_size(100.0);
/// turtle.forward(200.0);
+ /// # #[cfg(docs_images)] turtle_docs_helpers::save_docs_image(&turtle, "pen_thickness");
/// }
/// ```
///
@@ -891,6 +892,13 @@ impl Turtle {
}
}
+#[cfg(docs_images)]
+impl turtle_docs_helpers::SaveDocsSvg for Turtle {
+ fn save_docs_svg(&self, path: std::path::PathBuf) {
+ self.turtle.save_docs_svg(path);
+ }
+}
+
#[cfg(test)]
mod tests {
use super::*;
diff --git a/turtle_docs_helpers/Cargo.toml b/turtle_docs_helpers/Cargo.toml
new file mode 100644
index 0000000..4a6a129
--- /dev/null
+++ b/turtle_docs_helpers/Cargo.toml
@@ -0,0 +1,9 @@
+[package]
+name = "turtle_docs_helpers"
+version = "0.1.0"
+authors = ["Sunjay Varma <varma.sunjay@gmail.com>"]
+edition = "2018"
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies]
diff --git a/turtle_docs_helpers/src/lib.rs b/turtle_docs_helpers/src/lib.rs
new file mode 100644
index 0000000..40a9937
--- /dev/null
+++ b/turtle_docs_helpers/src/lib.rs
@@ -0,0 +1,17 @@
+use std::path::{Path, PathBuf};
+
+/// Saves the image being drawn as an SVG and panics if an error occurs
+///
+/// This is different from the `save_svg` method on `Drawing` and `AsyncDrawing`
+/// because this is only meant to be used for automation and may need to access
+/// internal APIs.
+pub trait SaveDocsSvg {
+ fn save_docs_svg(&self, path: PathBuf);
+}
+
+/// Saves the currently drawn image to `docs/assets/images/docs/{output_name}`
+pub fn save_docs_image<T: SaveDocsSvg>(drawing: &T, output_name: &str) {
+ let svg_path = Path::new("docs/assets/images/docs").join(output_name).with_extension("svg");
+ drawing.save_docs_svg(svg_path);
+ todo!()
+}Notes:
Please make sure you document in I think if you base your work on this, you should be able to get it working. Please don't hesitate to let me know if you have any more questions! |
|
@JosephLing I saw your messages on Zulip today and sent a reply. (Sorry for the delay!) Could you push the latest version of your code so I can see the issues you ran into trying to make PNGs from the SVG files? |
note: resvg is used over nsvg as it provides more accurate image dimensions
|
@JosephLing I just finished looking at the latest set of changes. Thanks for continuing to work on this! 😄 I noticed that the approach you're currently taking has deviated pretty significantly from what we've been discussing so far. Did you get a chance to look at the patch from my previous comment? The reason that approach works well is that it keeps this functionality largely separate and independent from the rest of the crate. That's important because these changes are just to automate a part of the crate maintanence. None of this should significantly alter any of the code that belongs to the main crate. Could you take a look at that patch again and base your changes off of the approach taken there? You should be able to take the Note: The
Hopefully that explains a few of the choices I made in structuring the code. Sorry if this stuff wasn't clear before! If you run into any major issues, I'm happy to help you figure out how to work around them, but in general I think that approach should work very well. I'm also hoping that now that you've gotten it working once it will not be a huge amount of effort for you to move your code over. Let me know if you have any questions! 😁 |
|
I saw the previous comment but wanted to try and go for a more automated approach so tried this out. But I will work on putting it back to the other way and making sure that:
Yep that was happening with
For For the images generated are they good? as if they are all okay they can be added to the docs by having them reference the master branch. One future issue/difficulty might be versioning the docs although I imagine there won't be any major changes or too many people stuck on a version. |
Thanks! At this point, it may be easiest to start from master, apply the patch, and then copy/paste the # Start on the docs_images_helper branch
git checkout docs_images_helper
# Backup your work
git branch docs_images_helper_backup
# Switch to master
git checkout master
# Add an upstream remote if you don't have one already
git remote add upstream https://github.com/sunjay/turtle.git
# Update to the latest master
git pull upstream master
# Remove the current version of the feature branch (don't worry, it is backed up!)
git branch -D docs_images_helper
# Create a new version of the feature branch based on the updated master
git checkout -b docs_images_helper
# Apply the patch
git apply pr190.patch
# ...finish the work...
# Push the branch
git push -fThis assumes you have the patch saved in a file called Alternatively, I've pushed the changes onto a # See above for explanations of these commands
git checkout docs_images_helper
git branch docs_images_helper_backup
git checkout master
git remote add upstream https://github.com/sunjay/turtle.git
git pull upstream master
# Important: delete the previous copy of the branch (it should be backed up to docs_images_helper_backup)
git branch -D docs_images_helper
# Fetch all the branches from upstream (git pull does this, so you might not need it)
git fetch upstream
# Checkout the pr190 branch but name it `docs_images_helper`
git checkout -b docs_images_helper upstream/pr190
# ...finish the work...
# Push the branch
git push -f
Thanks! I like the
Let's push this work to a future PR. I want to land this as soon as possible with just a basic amount of functionality. The nice thing about this is that it's hidden behind a
Yes, this is definitely extra tricky because docs.rs doesn't support image hosting at all. You'll notice that all of the images are referenced by their commit ID in the docs. We can work on solving that problem later. The hardest part is regenerating the images, so let's get that right first.
The images are good, though please do make sure you only add this for examples that don't need the turtle shown. I also noticed while reviewing that some of the images seem to be incorrect. It's worth checking if you accidentally forgot to add a call before a GitHub has a rich diff mode that should make the incorrect images very clear:
|
|
Hey @JosephLing, anything I can do to help you make progress with this? No worries if you won't have time to work on it anymore. We can take the awesome amount of progress you've made so far and try to finish it some other way if it comes to that. From the last comment I left it looks like there's just a few more steps to go! Appreciate all the time that you've put into this! |
|
Thanks for reaching out @sunjay, life and job hunting have consumed my time/energy recently. I might be able to work on it in a few weeks. But more realisticly if someone wants to take it over I can be around to help on zulip chat 👍 Thanks for all the help! and yep it's close to be finished 😃 |
|
No worries @JosephLing. I also went through a bunch of life and job hunting things recently, it can be really rough. 😰 Hope everything works out well for you! If I get a chance to work on this or if someone else volunteers, we'll pass on the work, but otherwise you are welcome to come back and finish this off anytime. Thanks again for all the time/effort you've put in so far! 🎉 |
For #179 this implements generating the svgs from the doc tests using
docs_imagefeature flag.There was one main slight complication in that a few examples required the user to click on the window. For those tests which where originally
no_runI split them up into separate doc tests but hopefully they should all be one coherent code example...is_visible, for the title bar I am not sure the best way to handle it but it is easy to get the data out.