Added the arc method to turtle with two examples #189

hans-pistor · 2020-08-05T18:36:28Z

Fixes #82

Expanded on @DeltaManiac's work in #100 now that #173 was merged. Added a step parameter to more closely resemble the Python implementation's circle method.

Also added a second dragon curve example using the arc method compared to without it.

sunjay

Thanks for taking the time to work on this @hans-pistor! You've done a great job on this feature! 🎉

Since it's quite a bit of code, there was lots for me to comment on and make suggestions for improvement. I've used the GitHub suggestions feature as much as possible, so you should be able to batch all the changes together and commit them at once. That should save you most of the work. After you've done that, please go back through and make sure you've addressed my comments.

A lot of the changes requested are my bad because the PR I asked you to base this on in #82 had some issues that I didn't notice back when I wrote the mentoring instructions. It's likely that we're going to need to rethink how the arc method is currently implemented. (See all my comments)

That's okay though! We'll work together to resolve all of that and get this merged as soon as possible! 😁

Please don't hesitate to let me know if you have any questions. 🙂

sunjay · 2020-08-05T18:41:50Z

examples/beats.rs

@@ -0,0 +1,43 @@
+extern crate turtle;


Suggested change

extern crate turtle;

extern crate is no longer necessary in the Rust 2018 edition.

sunjay · 2020-08-05T18:42:54Z

examples/dragon_circle.rs

@@ -0,0 +1,29 @@
+extern crate turtle;


Suggested change

extern crate turtle;

extern crate is no longer necessary in the Rust 2018 edition.

sunjay · 2020-08-05T18:44:21Z

src/async_turtle.rs

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


It looks like you have rustfmt running in your editor. Could you please remove all the changes that aren't relevant to the PR?

sunjay · 2020-08-05T18:50:35Z

src/turtle.rs

+    /// // Turtle will draw an arc of 90 in a clockwise direction in 100 steps
+    /// turtle.arc(100.0, Some(90.0), Some(100));
+    /// ```
+    pub fn arc(&mut self, radius: Distance, extent: Option<Angle>, steps: Option<i32>) {


Suggested change

pub fn arc(&mut self, radius: Distance, extent: Option<Angle>, steps: Option<i32>) {

#[cfg(feature = "unstable")] //TODO: Should we use the builder pattern here instead?

pub fn arc(&mut self, radius: Distance, extent: Option<Angle>, steps: Option<i32>) {

Let's make this method unstable for now. I am still questioning whether we should use Option in the parameters or maybe use the builder pattern instead. If we do take an Option type, we might want to take Into<Option> instead so you can pass in the numbers directly without having to wrap them in Some.

sunjay · 2020-08-05T19:00:52Z

examples/dragon_circle.rs

+fn main() {
+    let mut turtle = Turtle::new();
+    turtle.set_speed("instant");
+    for i in 1..100000 {


Looks like the image is getting cut off at the current window size. We should consider shifting the whole drawing up a bit (using pen_up, forward, and then pen_down before this loop). We may also want to decrease the number of iterations to something that fits inside the window and finishes in a reasonable time.

sunjay · 2020-08-05T21:18:25Z

src/turtle.rs

+    /// The `extent` parameter is a floating point number that represents how much you want the
+    /// turtle to rotate. If the `extent` parameter is `None` then it is defaulted to 360. If
+    /// a negative `extent` is passed the arc is drawn in the opposite direction.


Suggested change

/// The `extent` parameter is a floating point number that represents how much you want the

/// turtle to rotate. If the `extent` parameter is `None` then it is defaulted to 360. If

/// a negative `extent` is passed the arc is drawn in the opposite direction.

/// The `extent` parameter is an angle that represents how much of the circle to draw.

/// If `None`, the entire circle will be drawn. If negative, the arc will be drawn in

/// the opposite direction.

Instead of specifying 360, I used "the entire circle" since the actual default value should depend on what the current angle unit is set to.

sunjay · 2020-08-05T21:20:20Z

src/turtle.rs

+    /// the `steps` parameter is an integer that represent over how many steps you want the turtle to draw. If the `steps` parameter
+    /// is `None` then it is defaulted to the arc_length / 4.0.


Suggested change

/// the `steps` parameter is an integer that represent over how many steps you want the turtle to draw. If the `steps` parameter

/// is `None` then it is defaulted to the arc_length / 4.0.

/// The `steps` parameter represents how many steps to take while drawing the arc. If

/// `None`, the value will be determined automatically based on the extent and radius

/// of the arc.

We don't want to commit to how we're calculating the steps, so I modified this line to be a bit more vague.

sunjay · 2020-08-05T21:20:34Z

src/turtle.rs

+    /// the `steps` parameter is an integer that represent over how many steps you want the turtle to draw. If the `steps` parameter
+    /// is `None` then it is defaulted to the arc_length / 4.0.
+    ///
+    /// #Example


Suggested change

/// #Example

/// # Example

nit: whitespace

sunjay · 2020-08-05T21:30:18Z

src/turtle.rs

+    ///
+    /// #Example
+    ///
+    /// ```rust


Suggested change

/// ```rust

/// ```rust,no_run

Since there are no assertions in this example, it can probably be marked as no_run

sunjay · 2020-08-05T21:32:19Z

src/turtle.rs

+    /// // Turtle will draw an arc of 90 in a clockwise direction
+    /// turtle.arc(100.0, Some(90.0), None);
+    ///
+    /// // Turtle will draw an arc of 90 in a anti-clockwise direction
+    /// turtle.arc(-100.0, Some(90.0), None);
+    ///
+    /// // Turtle will draw an arc of 90 in a clockwise direction
+    /// turtle.arc(-100.0, Some(-90.0), None);
+    ///
+    /// //Turtle will draw an arc of 90 in a anti-clockwise direction
+    /// turtle.arc(-100.0, Some(90.0), None);
+    ///
+    /// // Turtle will draw an arc of 90 in a clockwise direction in 10 steps
+    /// turtle.arc(100.0, Some(90.0), Some(10));
+    ///
+    /// // Turtle will draw an arc of 90 in a clockwise direction in 100 steps
+    /// turtle.arc(100.0, Some(90.0), Some(100));


Suggested change

/// // Turtle will draw an arc of 90 in a clockwise direction

/// turtle.arc(100.0, Some(90.0), None);

///

/// // Turtle will draw an arc of 90 in a anti-clockwise direction

/// turtle.arc(-100.0, Some(90.0), None);

///

/// // Turtle will draw an arc of 90 in a clockwise direction

/// turtle.arc(-100.0, Some(-90.0), None);

///

/// //Turtle will draw an arc of 90 in a anti-clockwise direction

/// turtle.arc(-100.0, Some(90.0), None);

///

/// // Turtle will draw an arc of 90 in a clockwise direction in 10 steps

/// turtle.arc(100.0, Some(90.0), Some(10));

///

/// // Turtle will draw an arc of 90 in a clockwise direction in 100 steps

/// turtle.arc(100.0, Some(90.0), Some(100));

/// // Both of these lines will cause the turtle to draw

/// // a 90 degree clockwise arc (by turning right)

/// turtle.arc(100.0, Some(90.0), None);

/// turtle.arc(-100.0, Some(-90.0), None);

///

/// // Both of these lines will cause the turtle to draw

/// // a 90 degree counterclockwise arc (by turning left)

/// turtle.arc(-100.0, Some(90.0), None);

/// turtle.arc(100.0, Some(-90.0), None);

///

/// // A 90 degree clockwise arc in 10 steps

/// turtle.arc(100.0, Some(90.0), Some(10));

/// // A 90 degree clockwise arc in 100 steps

/// turtle.arc(100.0, Some(90.0), Some(100));

When using an example to try to teach the subtleties of an API to someone, it's good to group related statements together so readers can see which small differences lead to the same outcome. I've changed this example to group some of the lines you added together so the user can see how the signs of the radius and extent affect the output.

Note: If your other documentation about us taking the absolute value of the radius is true, are these examples even accurate? I think this should be fine once we fix the code to no longer take the absolute value. We should definitely make sure we're accurate about the behaviour of our code in our examples!

sunjay · 2020-08-26T18:21:59Z

@hans-pistor Anything I can do to help you move forward on this? No worries if you just haven't had time or won't get a chance to keep working on this. Just want to make sure I offer in case there's anything I can do. :)

sunjay · 2020-09-05T19:13:59Z

Hi @hans-pistor, I'm going to close this PR for now since it hasn't been updated in a while. You are welcome back anytime to keep working on this or to work on something else. No worries if something got in the way here this time.

Appreciate the time and effort you put into this! 😊

Added the arc method to turtle with two examples

e3b9607

sunjay self-requested a review August 5, 2020 18:39

sunjay requested changes Aug 5, 2020

View reviewed changes

sunjay closed this Sep 5, 2020

sunjay mentioned this pull request Sep 5, 2020

Add arc method to Turtle #82

Open

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the arc method to turtle with two examples #189

Added the arc method to turtle with two examples #189

hans-pistor commented Aug 5, 2020 •

edited by sunjay

Loading

sunjay left a comment

sunjay Aug 5, 2020

sunjay Aug 5, 2020

sunjay Aug 5, 2020

sunjay Aug 5, 2020

sunjay Aug 5, 2020

sunjay Aug 5, 2020

sunjay Aug 5, 2020

sunjay Aug 5, 2020

sunjay Aug 5, 2020

sunjay Aug 5, 2020

sunjay commented Aug 26, 2020

sunjay commented Sep 5, 2020

	pub fn arc(&mut self, radius: Distance, extent: Option<Angle>, steps: Option<i32>) {
	#[cfg(feature = "unstable")] //TODO: Should we use the builder pattern here instead?
	pub fn arc(&mut self, radius: Distance, extent: Option<Angle>, steps: Option<i32>) {

		/// the `steps` parameter is an integer that represent over how many steps you want the turtle to draw. If the `steps` parameter
		/// is `None` then it is defaulted to the arc_length / 4.0.

-    /// the `steps` parameter is an integer that represent over how many steps you want the turtle to draw. If the `steps` parameter
-    /// is `None` then it is defaulted to the arc_length / 4.0.
+    /// The `steps` parameter represents how many steps to take while drawing the arc. If
+    /// `None`, the value will be determined automatically based on the extent and radius
+    /// of the arc.

Added the arc method to turtle with two examples #189

Added the arc method to turtle with two examples #189

Conversation

hans-pistor commented Aug 5, 2020 • edited by sunjay Loading

sunjay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunjay commented Aug 26, 2020

sunjay commented Sep 5, 2020

hans-pistor commented Aug 5, 2020 •

edited by sunjay

Loading