Skip to content
This repository was archived by the owner on Aug 30, 2025. It is now read-only.

Outputting stars into a properly structured markdown file #26

Open
Mordax wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Outputting stars into a properly structured markdown file #26

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 18 additions & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ use chrono::{DateTime, Utc};
use reqwest::header::{qitem, Accept, Authorization, Bearer, Link, RelationType, UserAgent};
use serde_derive::Deserialize;
use std::{env, error, fmt, mem};
use std::error::Error;
use std::io::prelude::*;
use std::fs::File;
use std::path::Path;

#[derive(Debug)]
pub struct Config {
Expand Down Expand Up @@ -128,6 +132,20 @@ pub fn collect_stars(config: Config) -> Result<(), Box<dyn error::Error>> {
}
println!("Collected {} stars", stars.len());

let path = Path::new("output.md");
let display = path.display();

let mut file = match File::create(&path) {
Copy link
Owner

@0xazure 0xazure Nov 13, 2018

Choose a reason for hiding this comment

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

Since path isn't used anywhere else, we can inline its value; File::create can accept as an argument anything that implements the AsRef<Path> trait which str does.

See the File::create example in the docs.

Err(why) => panic!("couldn't create {}: {}",
Copy link
Owner

@0xazure 0xazure Nov 13, 2018

Choose a reason for hiding this comment

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

panic! is generally a last resort, since it causes the program to terminate immediately and generally doesn't print user-friendly output (you can see an example of the unfriendliness in this Rust playground)

Instead of matching this failure here, we should communicate this failure back into main() by returning the Resulting Error from the current function. The structure in supernova follows almost exactly the example CLI tool from Chapter 12.3 of the Rust Book (2nd Ed.), which shows how to refactor a panic!-based error approach into printing user-friendly errors, so take a look and see how the example in the Book matches up with the current implementation.

I'd recommend having a look at the try! macro and the ? operator for different ways to deal with errors in Rust.

display,
why.description()),
Copy link
Owner

@0xazure 0xazure Nov 13, 2018

Choose a reason for hiding this comment

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

I had to go looking to find the description() method, I don't think I've ever actually seen it before!

It's defined on the std::error::Error trait, and is apparently soft-deprecated and not recommended for use.

Ok(file) => file,
};

for star in stars.iter(){
Copy link
Owner

@0xazure 0xazure Nov 13, 2018

Choose a reason for hiding this comment

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

This introduces a new loop to print each star the the buffer, but there is already a loop for printing each star to stdout on L130 which means this isn't very efficient because we're looping the vec twice instead of once.

write!(file, "{}\x20\x20\r", star);
Copy link
Owner

@0xazure 0xazure Nov 13, 2018

Choose a reason for hiding this comment

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

I'm having trouble understanding the format string here, what's the purpose of \x20\x20? I'm not super familiar with string literals, is \x20 a SPACE?

Also, careful with your line terminators, \r is CARRIAGE RETURN and is platform-specific to Windows, other operating systems only use \n to denote the end of a line.

I see you're making use of write!, is there any preference for this over writeln!?

Copy link
Contributor Author

@Mordax Mordax Nov 14, 2018

Choose a reason for hiding this comment

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

\x20 is indeed a space - in order to get the markdown on the next line rather than in the same line, it needs 2 spaces and a carriage return/newline.

\r is a Rust token: https://doc.rust-lang.org/reference/tokens.html - but if it does fail using that on linux then writeln! may be better if it always makes sure there's a newline regardless of OS.

Copy link
Owner

@0xazure 0xazure Nov 15, 2018

Choose a reason for hiding this comment

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

to get the markdown on the next line rather than in the same line, it needs 2 spaces and a carriage return/newline

Huh, a close reading of the original Markdown spec does specify 2 spaces before a return.

I'm not actually aware of any other specs that specify this - GitHub's Markdown spec says "A line is a sequence of zero or more characters other than newline (U+000A) or carriage return (U+000D), followed by a line ending or by the end of file" which doesn't mention spaces at all - and requiring 2 spaces will likely trigger trailing whitespace warnings in many editors.

\r is a Rust token

Yup! Though \r is a standard escape sequence so it's not Rust-specific. The problem is on Windows pressing the ENTER key generates the key sequence \r\n whereas on most other operating systems ENTER results in just \n. Microsoft only got around to adding support for Unix/Linux line endings in Nodepad this year. You can use write! and add the linefeed yourself, but writeln! does that for you so it's up to you which one you'd like to use!

}

Ok(())
}

Expand Down