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

Outputting stars into a properly structured markdown file #26

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@Mordax
Contributor

Mordax commented Nov 10, 2018

Wanted to actually write some Rust code, so I added an extra step that outputs the stars into a nicely structured output.md. Closes #11 .

Let me know if I should adjust anything. Cheers!

@Mordax Mordax changed the title from Outputing stars into a properly structured markdown file to Outputting stars into a properly structured markdown file Nov 10, 2018

@SeanPrashad

This comment has been minimized.

Contributor

SeanPrashad commented Nov 10, 2018

Nice! Would you be able to link to the respective issue for this feature?

@Mordax

This comment has been minimized.

Contributor

Mordax commented Nov 10, 2018

Yes, sorry!

@0xazure

Hey @Mordax, thanks for taking a crack at fixing our malformed Markdown output!

I've made some inline comments, let me know if you have any questions. Dealing with errors in Rust definitely takes some getting used to, so I tried to provide links to further reading you might be interested in checking out.

From a design standpoint, I'm not sure I like the idea of creating a separate file to write out the results. supernova should play nicely in a *nix ecosystem, so we definitely want to preserve the ability to pipe the output to various destinations. However, without reading the code it's not obvious that supernova will create a file, and in this implementation the user isn't able to control either the location or the name of the created file. Since File::create truncates existing files this could be really bad for an unsuspecting user because it could cause data loss.

Given that, I'm not sure yet how we should move forward with this. Let me know what you think, and I'd be really interested to talk about the design with you!

Ok(file) => file,
};
for star in stars.iter(){

This comment has been minimized.

@0xazure

0xazure Nov 13, 2018

Owner

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.

};
for star in stars.iter(){
write!(file, "{}\x20\x20\r", star);

This comment has been minimized.

@0xazure

0xazure Nov 13, 2018

Owner

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!?

This comment has been minimized.

@Mordax

Mordax Nov 14, 2018

Contributor

\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.

This comment has been minimized.

@0xazure

0xazure Nov 15, 2018

Owner

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!

let path = Path::new("output.md");
let display = path.display();
let mut file = match File::create(&path) {

This comment has been minimized.

@0xazure

0xazure Nov 13, 2018

Owner

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.

let mut file = match File::create(&path) {
Err(why) => panic!("couldn't create {}: {}",
display,
why.description()),

This comment has been minimized.

@0xazure

0xazure Nov 13, 2018

Owner

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.

let display = path.display();
let mut file = match File::create(&path) {
Err(why) => panic!("couldn't create {}: {}",

This comment has been minimized.

@0xazure

0xazure Nov 13, 2018

Owner

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.

@0xazure

This comment has been minimized.

Owner

0xazure commented Nov 23, 2018

Hey @Mordax, haven't heard back from you about the review comments yet. Let me know if you have any questions, I think we should talk about the direction for this pull request as I mentioned in the review.

@Mordax

This comment has been minimized.

Contributor

Mordax commented Nov 23, 2018

@0xazure Yes, I know, sorry about that. I'm being pulled in many directions at the moment, and I'll get to as soon as I can.

@Mordax

This comment has been minimized.

Contributor

Mordax commented Nov 30, 2018

It may be prudent to put this PR on hold until we resolve #33.

@0xazure

This comment has been minimized.

Owner

0xazure commented Dec 3, 2018

I'm not sure #33 is relevant, since it's the functionality that should be driving the UI rather than the other way around.

It would be great if we could get this merged, but I definitely agree we need to think about how we expose functionality to the user beyond simply changing how the output is written to its destination.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment