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

Formatting of Code in Book is Sometimes Inconsistent with rustfmt #1690

Open
JosephTLyons opened this issue Dec 15, 2018 · 14 comments

Comments

@JosephTLyons
Copy link
Contributor

@JosephTLyons JosephTLyons commented Dec 15, 2018

I've been reading the Rust book much more recently and have come to notice that some examples do not follow the way in which rustfmt formats code. For example, in the Method Syntax chapter of the book, an instance of the Rectangle struct is formatted as such:

let rect1 = Rectangle { width: 30, height: 50 };

but rustfmt would format it as such:

let rect1 = Rectangle {
        width: 30,
        height: 50,
};

I've found other discrepancies between the book's formatting and rustfmt. Rustfmt seems to be a tool that was made to help Rustaceans put their code in the exact format agreed upon by the creators, so I think it would be fit to format the code in the book with rustfmt. If there is any interest in this happening, but the task is considered large, I would be happy to help.

@steveklabnik

This comment has been minimized.

Copy link
Member

@steveklabnik steveklabnik commented Dec 15, 2018

The intention is to follow rustfmt's defaults, for sure. I'd love to accept some PRs to fix this!

Ideally, we'd have some way to be able to format them all automatically...

@JosephTLyons JosephTLyons changed the title Formatting of Code in Book is inconsistent with rustfmt Formatting of Code in Book is Inconsistent with rustfmt Dec 15, 2018
@JosephTLyons

This comment has been minimized.

Copy link
Contributor Author

@JosephTLyons JosephTLyons commented Dec 15, 2018

I will get on formatting some chapters with rustfmt and submitting them. It would be great to have a tool to do it, although not sure how to do it with the code being mixed in markdown files.

@JosephTLyons JosephTLyons changed the title Formatting of Code in Book is Inconsistent with rustfmt Formatting of Code in Book is, at Times, Inconsistent with rustfmt Dec 15, 2018
@JosephTLyons JosephTLyons changed the title Formatting of Code in Book is, at Times, Inconsistent with rustfmt Formatting of Code in Book is Sometimes Inconsistent with rustfmt Dec 15, 2018
@JosephTLyons

This comment has been minimized.

Copy link
Contributor Author

@JosephTLyons JosephTLyons commented Dec 15, 2018

After inspecting some chapters, I don’t think it would be easy to make a tool to automate the process, as there specifically are examples within the book of how code should not look. Unless there was some sort of flagging system to flag those code chunks, they would be corrected. Although tedious, I think manual corrections will be the way to go.

@steveklabnik

This comment has been minimized.

Copy link
Member

@steveklabnik steveklabnik commented Dec 15, 2018

@codesections

This comment has been minimized.

Copy link

@codesections codesections commented Dec 17, 2018

I'd be happy to help with this. I'm very new to rust, but catching formatting issues is one way that even I can contribute—and free up other for more substantive work.

I took a look at it and I think it's possible to partly automate it with vim macros. Using that method, I was able to check that there aren't any issues in Ch1 and find a few minor changes Ch2. (All the changes I found were either adding line-breaks or alphabetizing the use statements.) Would you like me to submit a PR with the Ch2 changes and then submit additional PRs for subsequent chapters? Or would you prefer that I submit a single WIP PR and push additional commits as I go through the chapters?

@JosephTLyons

This comment has been minimized.

Copy link
Contributor Author

@JosephTLyons JosephTLyons commented Dec 17, 2018

This task is a bit more challenging that I initially expected. There are various aspects to consider. For instance, there seems to be some things rustfmt corrects that seem trivial in the context of the book. For instance, in one of the first chapters, we are given an outline of a function:

fn main() {
}

but rustfmt formats it as such:

fn main() {

}

So I encounter these issues where I'm not sure if I should just entirely allow all formatting changes by rustfmt just to be consistent and to entirely keep the process as automate-able as possible with no personal opinion injected into it, or do I keep it from changing trivial things like this? What is your opinion @steveklabnik ?

Other things you have to consider is this isn't just about formatting code and moving on, you have to read the text that refers to that code to see if you need to adjust what is being said that may no longer be correct; I've had to do this already in a spot or two, as the changes made the text following it inaccurate. All of this to say that this isn't a quick thing.

As mentioned by @codesections , most of the changes in the first few chapters are line breaks and alphabetizing use statements. I'm currently working on The Guessing Game Tutorial, this is the part of the book where code changes become very significant.

Another thing we have to consider is how often we apply rustfmt to sections that have already been formatted? I think it would be maddening to have to do this with every tiny release of rustfmt, so maybe there could be some sort of guide for how often it is done. For example, it would only be reformatted again if there was a new major version of rustfmt (1.X.X -> 2.X.X), and it wouldn't be reformatted for every patch or minor version of rustfmt released.

Last question, @steveklabnik , would you be against the idea of me creating a simple .md file to put into the repo to keep track of the chapters that have been formatted with X version of rustfmt? So others wanting to jump on the issue can know what is already done? I could whip up a table fairly quickly. If so, where would such a temp doc go?

Here's a sample:

screen shot 2018-12-17 at 5 05 48 pm

@steveklabnik

This comment has been minimized.

Copy link
Member

@steveklabnik steveklabnik commented Dec 18, 2018

So, one interesting and related thing: https://rust-lang-nursery.github.io/mdBook/format/mdbook.html#including-files

I wonder if we could do something where we pull the code examples out into their own files, and then we'd be able to rustfmt those. This would also maybe help with the error messages bit too, because we could also have something where it writes the error messages out to files...

@carols10cents what do you think about that?

in one of the first chapters, we are given an outline of a function:

Yeah, that's tricky...

would you be against the idea of me creating a simple .md file to put into the repo to keep track of the chapters that have been formatted with X version of rustfmt?

I'm not opposed. Carol?

@codesections

This comment has been minimized.

Copy link

@codesections codesections commented Dec 18, 2018

@JosephTLyons if you put together a table, just let me know how/where I can help. I'm very new to rust and will happily defer to any of y'all, but would be happy to be helpful without duplicating work others have done.

@carols10cents

This comment has been minimized.

Copy link
Member

@carols10cents carols10cents commented Dec 18, 2018

I wonder if we could do something where we pull the code examples out into their own files, and then we'd be able to rustfmt those. This would also maybe help with the error messages bit too, because we could also have something where it writes the error messages out to files...

@carols10cents what do you think about that?

OOH! I didn't know mdbook could do that! We need to provide standalone code for the examples too, so win-win-win!

@carols10cents

This comment has been minimized.

Copy link
Member

@carols10cents carols10cents commented Dec 18, 2018

would you be against the idea of me creating a simple .md file to put into the repo to keep track of the chapters that have been formatted with X version of rustfmt?

I'm not opposed. Carol?

If we can go this route of extracting the code into files, then we should be able to run rustfmt on everything pretty easily, right?

Ugh one wrinkle might be the mdbook hiding with # that we do, hmmmm...

@JosephTLyons

This comment has been minimized.

Copy link
Contributor Author

@JosephTLyons JosephTLyons commented Dec 18, 2018

I have a table done and will have it ready to go as soon as I get some direction of where to place it in this repo :)

@JosephTLyons

This comment has been minimized.

Copy link
Contributor Author

@JosephTLyons JosephTLyons commented Dec 18, 2018

@carols10cents I noticed that a lot of code doesn’t instantly format with rustfmt. Many code chunks are not complete pieces of code, missing the main function and such. I had to manually copy and paste code into another more complete file to run rustfmt many times (being extremely careful when copying back only to include original code ((heavily checking diffs)). So I’m not entirely sure how much easier putting the code into their own files will make it. I know it will remove the markdown around them, but it will still require a lot of manual setup to run rustfmt I think.

@carols10cents

This comment has been minimized.

Copy link
Member

@carols10cents carols10cents commented Dec 19, 2018

@carols10cents I notirced that a lot of code doesn’t instantly format with rustfmt. Many code chunks are not complete pieces of code, missing the main function and such.

We actually want to extract examples into runnable files so that we have a directory of just the code in the book that people can run without having to type in, and to reference if they do type it in and theirs doesn't work. The "include" mdbook feature looks like it lets you specify which lines from a file you'd like to include, which might work for, say, putting fn main {} in the file but not pulling that in to the text. It also looks like there might be another way to include partial files soon.

I started trying out pulling code into files in this branch. Unfortunately, I want to also retain mdbook test functionality, and I'm getting an error now that makes it look like the include preprocessing doesn't happen before mdbook test, going to file a bug now but I'm not sure all of this work to do rustfmt is worth it unless we can rerun it automatically, which seems like it needs external files.

@JosephTLyons

This comment has been minimized.

Copy link
Contributor Author

@JosephTLyons JosephTLyons commented Dec 19, 2018

Understandable. I won't waste the maintainer's time by opening a PR then for the new table I created, but I'll leave a link here in case you or Steve decide you'd like to use it for either the automatic or manual formatting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.