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

adding a write example #226

Merged
merged 16 commits into from
Mar 31, 2021
Merged

adding a write example #226

merged 16 commits into from
Mar 31, 2021

Conversation

JuanMarchetto
Copy link
Contributor

@JuanMarchetto JuanMarchetto commented Feb 22, 2021

Hi, i'm creating this example that draw any text you want.
For example, if you run in console:
cargo run --example write hello
it will draw "Hello"
if you don't introduce any text:
cargo run --example write
it will draw "Hello, World!"
Captura de pantalla de 2021-02-21 19-56-03

you can also pass a font size as a second parameter:
cargo run --example write "Hello, World!" 50
and all the letters will ajust to that size, if you don't pass that second parameter 20.0 will be the default.

I still need to implement some more letter but i like to show you the progress at this point.
At this point if you introduce a character that currently doesn't have an implemtations will we replaced with '?' and in the console it will trow the followinig message:

Captura de pantalla de 2021-02-21 20-59-59

I hope you like it.

@codecov
Copy link

codecov bot commented Feb 22, 2021

Codecov Report

Merging #226 (bf3e244) into master (3469f15) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #226   +/-   ##
=======================================
  Coverage   61.17%   61.17%           
=======================================
  Files          40       40           
  Lines        2697     2697           
=======================================
  Hits         1650     1650           
  Misses       1047     1047           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3469f15...bf3e244. Read the comment docs.

@sunjay
Copy link
Owner

sunjay commented Feb 22, 2021

This is a very cool idea @JuanMarchetto! Great work! 😁 🎉

I'm glad you chose to submit this early, as big examples like this often need a lot of feedback, so discussing it earlier is always better than later. Lots of code to review!

I won't have a chance to look at this in detail for a few days, but here's a couple of things I noticed from skimming through the code:

  • It looks like the changes from your clock PR made it into this one for some reason. I'd guess that your local master branch is out of date. I recommend rebasing your PR to fix that. (As a general note, it's a good idea to look through the "Files changed" tab on your PR to catch this kind of stuff earlier.)
  • Could you rename your example to letters instead of write? It's possible we'll add a write method in the future (Text support #22) and I won't want this to conflict with the examples we'll eventually add for that
  • Please run rustfmt on the files you're adding so the formatting is consistent throughout them
    • Our codebase isn't ready to use with cargo fmt yet, so please use the rustfmt tool on your individual files only:
      rustfmt examples/letters/*
      
  • Since letters.rs is going to get pretty big with 26+ letters, it's a good idea to split it up further into modules like a_to_d.rs, e_to_h.rs, etc.
  • Instead of other_chars.rs, the name punctuation.rs might fit better for what's there now
  • Good idea to include the ¡ character! I think that's great! 😁
  • Since char is the name of a type in Rust, it's a good idea to avoid using it as a variable name. Usually people go with ch or character or something.
  • I noticed that you're using radians instead of degrees. Is there any reason for that? I think it might be easier to read / contribute to your code if you use degrees instead
  • At the top of src/write/main.rs (which you'll rename to src/letters/main.rs), please add a short comment (example) that describes what this example does and how to run it. This will help people understand what this code does without having to read all of it.
  • I quickly put together some argument parsing code that handles a bunch of edge cases your current code doesn't and also prints out a nice help argument whenever there is an error or when the user passes in --help. Since you're learning Rust, I figured this would be a good example of a nice way to write this code using pattern matching and iterators. For actual production applications people usually use structopt (no need for that here). Please incorporate this into your example when you have a chance. 😊
Argument Parsing Code
use std::env;
use std::process;

fn main() {
    let (text, font_size) = parse_args();
    
    println!("{}", text);
    println!("{}", font_size);
}

/// Parses the command line arguments or exits with a help message if there was
/// an error
fn parse_args() -> (String, f64) {
    let mut args = env::args();
    
    // Skip the first argument (the executable name)
    args.next();
    
    // First argument is the text to draw
    let text = match args.next() {
        Some(text) if text == "--help" => print_help(),
        
        // This can produce any text, including the empty string
        Some(text) => text,
        // Default to `Hello, World!`
        None => "Hello, World!".to_string(),
    };
    
    // Second argument is the font size
    let font_size: f64 = match args.next() {
        Some(text) if text == "--help" => print_help(),
        
        Some(font_size) => match font_size.parse() {
            Ok(font_size) => if font_size >= 1.0 {
                font_size
            } else {
                println!("Font size argument must be at least 1.0");
                println!();
                print_help();
            },
            
            Err(err) => {
                println!("Font size argument must be a valid number: {}", err);
                println!();
                print_help();
            },
        },
        
        // Default to a font size of 20
        None => 20.0,
    };
    
    // Not expecting any other arguments
    if args.next().is_some() {
        print_help()
    }
    
    (text, font_size)
}

/// Prints the help message and then exits
///
/// `!` is the "never type" and it means this function never returns
fn print_help() -> ! {
    println!("Draws text on the screen using the turtle.");
    println!();
    println!("EXAMPLES:");
    println!("  cargo run --example letters 'Wow! So cool!'");
    println!("    Draw the text 'Wow! So cool!'");
    println!();
    println!("  cargo run --example letters 'Big text!' 50");
    println!("    Draw the text 'Big text!' with font size 50");
    println!();
    println!("  cargo run --example letters -- --help");
    println!("    Show this help information");
    println!();
    println!("FLAGS:");
    println!("  --help  show this help information");
    
    process::exit(0)
}

Overall, really great work with this PR! It might take a little longer than last time to review and get merged since there is much more code, but I'm really looking forward to working with you to get this finished. Thanks for taking the time to work on this! 🥳 🎉

@sunjay sunjay self-requested a review February 22, 2021 02:19
@enaut enaut mentioned this pull request Feb 25, 2021
9 tasks
@sunjay
Copy link
Owner

sunjay commented Feb 27, 2021

Hey @JuanMarchetto I saw you pushed a new commit. Let me know when you're ready for a review. If you think this PR is ready, you can convert it from a draft PR to a regular PR.

@JuanMarchetto
Copy link
Contributor Author

Hey @JuanMarchetto I saw you pushed a new commit. Let me know when you're ready for a review. If you think this PR is ready, you can convert it from a draft PR to a regular PR.

Hey @sunjay this is not ready yet, I've been busy this week, but the next one I'll will get back to this and let you know when is ready. Thanks!

@sunjay
Copy link
Owner

sunjay commented Feb 27, 2021

Sounds good! Take as much time as you need. Just wanted to make sure you weren't waiting on me. :)

Happy to help out with anything if you run into any issues. Thanks for all your work!

@JuanMarchetto JuanMarchetto marked this pull request as ready for review March 3, 2021 21:35
@JuanMarchetto
Copy link
Contributor Author

@sunjay i made some progress here, i don't know how to address the coverage failing status do.
and if you what this is ready to rewiev. thanks!

@sunjay
Copy link
Owner

sunjay commented Mar 4, 2021

@JuanMarchetto Great! I will have a look over a few days. If I don't get back to you by next week please don't hesitate to ping me. (Hoping to have it done much sooner than that, but thought I'd mention it just in case.)

Don't worry about the coverage status. Only the required statuses + an approval from me are needed to get this merged. :)

@JuanMarchetto
Copy link
Contributor Author

@JuanMarchetto Great! I will have a look over a few days. If I don't get back to you by next week please don't hesitate to ping me. (Hoping to have it done much sooner than that, but thought I'd mention it just in case.)

Don't worry about the coverage status. Only the required statuses + an approval from me are needed to get this merged. :)

Hi @sunjay,
can you have a look to this PR?
Thanks!

@sunjay
Copy link
Owner

sunjay commented Mar 16, 2021

Hi @sunjay,
can you have a look to this PR?
Thanks!

Hey @JuanMarchetto! Sorry for the delay. I saw that you were still pushing things and assumed that you were still working on it. I will take a look over the next few days. Thanks for all your effort! 😁 🎉

@sunjay
Copy link
Owner

sunjay commented Mar 23, 2021

Hey! Really sorry for not getting to your PR yet. Going through some things and will try to find time soon. I usually try to be really quick with these things, but it unfortunately didn't work out this time. I will get to this! :)

@JuanMarchetto
Copy link
Contributor Author

Hey! Really sorry for not getting to your PR yet. Going through some things and will try to find time soon. I usually try to be really quick with these things, but it unfortunately didn't work out this time. I will get to this! :)

Hey sunjay, don't worry, i hope you are fine!

Copy link
Owner

@sunjay sunjay left a comment

Choose a reason for hiding this comment

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

This is a very cool example @JuanMarchetto! Great work! I really appreciate all the work you put into getting every letter/punctuation/special character working. You did an excellent job. 😁

image

Thanks for your patience with me! Just need to update from the master branch and this should be good to go. Glad to see this getting merged.

@sunjay sunjay merged commit 60dba16 into sunjay:master Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants