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

[Merged by Bors] - Code validation and format checking #281

Closed
wants to merge 9 commits into from
Closed

[Merged by Bors] - Code validation and format checking #281

wants to merge 9 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Feb 15, 2022

Objective

Migrates #203
Closes #83

Solution

  • Add code validation
  • Add code format checking Removed to unblock the code validation changes

Problems

During the implementation of the code validation and the format checking I encountered some problems which I would appreciate guidance or help with.

Need to be addressed

  1. Zola doesn't highlight the code blocks using the rust syntax if you add more than just the language to the code block attributes. Example rust highlights the code correctly, rust,no_run doesn't. So currently the code highlighting is broken. Zola is order dependent for attributes (as noted by @mockersf). So changing the attribute order from rust,no_run to no_run,rust works just fine.
  2. Running cargo fmt doesn't format the doc examples. Adding the rustfmt.toml option format_code_in_doc_comments = true enables this, but requires nightly. In my tests it also only works if you add the code examples directly without using #[doc = include_str!("_index.md")]. So currently the format checking is not working.

Nice to have

  1. The lib.rs file of the code-validation crate has to be manually updated each time a new _index.md file is added or removed. It might be a good idea to dive deeper and see if we can implement a solution where this work is done automatically. For now this should be more than fine though.

@ghost ghost added the help wanted label Feb 15, 2022
@alice-i-cecile
Copy link
Member

Zola doesn't highlight the code blocks using the rust syntax if you add more than just the language to the code block attributes. Example rust highlights the code correctly, rust,no_run doesn't. So currently the code highlighting is broken.

I'm not seeing an existing issue for this, but the author has been very responsive in the past. Let me make one and see what we can do.

@mockersf
Copy link
Member

mockersf commented Feb 15, 2022

if I remember correctly, Zola is order dependant for attributes and it should work with no_run,rust

@ghost
Copy link
Author

ghost commented Feb 15, 2022

That did the trick. Thank you!

This is important to know for everyone that creates and/or edits code examples. Do we have a place to note this down?

@alice-i-cecile
Copy link
Member

Do we have a place to note this down?

IMO we should have a CONTRIBUTING.md for this repo. Can you start one in this PR and note that down?

@alice-i-cecile
Copy link
Member

So, the question is: do we want to merge this as is, or block on a code formatting solution. IMO code formatting is the much-less important problem here; and I'd prefer to get this rolling faster to improve book quality and contributor experience.

@mockersf
Copy link
Member

  1. Running cargo fmt doesn't format the doc examples. Adding the rustfmt.toml option format_code_in_doc_comments = true enables this, but requires nightly. In my tests it also only works if you add the code examples directly without using #[doc = include_str!("_index.md")]. So currently the format checking is not working.

Would it work if you expand the macro then run the formatting?

@ghost
Copy link
Author

ghost commented Feb 16, 2022

So, the question is: do we want to merge this as is, or block on a code formatting solution. IMO code formatting is the much-less important problem here; and I'd prefer to get this rolling faster to improve book quality and contributor experience.

Agreed. I sadly couldn't find anything online about the format_code_in_doc_comments setting that verified my findings. Having CI check formatting without ever failing is not the worst thing ever, but if we want to merge this we could also remove the CI task and add it once we find a solution.

Would it work if you expand the macro then run the formatting?

Do you mean expanding the macro so that you get #[doc = "the actual string of the file"]? If that's the case then it didn't work for me. Seems like #[doc = "..."] is not respected by cargo fmt which seems weird, but format_code_in_doc_comments is an unstable feature so who knows.

@cart cart removed the help wanted label Mar 10, 2022
@alice-i-cecile
Copy link
Member

@KDecay IMO we should split out the validation and formatting checks in order to unblock this.

I really really want this in place before I (or others) attempt to write more content for the book.

@ghost
Copy link
Author

ghost commented Mar 10, 2022

Definitely agreed. I'll update the CI workflow so it only validates the code.

@ghost ghost marked this pull request as ready for review March 10, 2022 21:44
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 10, 2022

This no longer closes #279, however it does close #83.

@ghost
Copy link
Author

ghost commented Mar 10, 2022

Updated the description accordingly.

@alice-i-cecile alice-i-cecile added this to the New Book Launch milestone Mar 11, 2022
@IceSentry
Copy link
Contributor

So, just to make sure I understand this correctly. Let's say I want to have a small code block that only shows a few lines that are added to a block that was defined earlier, I would now need to have the entire valid program in the code block and hide all the lines I don't want to show?

@ghost
Copy link
Author

ghost commented Mar 11, 2022

@IceSentry Yup. This way we ensure that the code shown to the user is valid. You can also use the ignore tag, but that would defeat the purpose of this change.

@IceSentry
Copy link
Contributor

Right, makes sense, where can I get more information on how the hide-line parameter works?

@alice-i-cecile
Copy link
Member

It looks like it's an extension of the rust-doc feature, but I can't immediately see where the hide-line parameter can be found.

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Mar 11, 2022
# Objective

Migrates #203
Closes #83

# Solution

- [x] Add code validation
- [x] ~~Add code format checking~~ Removed to unblock the code validation changes

# Problems

During the implementation of the code validation and the format checking I encountered some problems which I would appreciate guidance or help with.

## Need to be addressed

1. ~~Zola doesn't highlight the code blocks using the rust syntax if you add more than just the language to the code block attributes. Example `rust` highlights the code correctly, `rust,no_run` doesn't. So currently the code highlighting is broken.~~ Zola is order dependent for attributes (as noted by @mockersf). So changing the attribute order from `rust,no_run` to `no_run,rust` works just fine.
1. Running `cargo fmt` doesn't format the doc examples. Adding the `rustfmt.toml` option `format_code_in_doc_comments = true` enables this, but requires nightly. In my tests it also only works if you add the code examples directly without using `#[doc = include_str!("_index.md")]`. So currently the format checking is not working.

## Nice to have

1. The `lib.rs` file of the `code-validation` crate has to be manually updated each time a new `_index.md` file is added or removed. It might be a good idea to dive deeper and see if we can implement a solution where this work is done automatically. For now this should be more than fine though.

Co-authored-by: KDecay <KDecayMusic@protonmail.com>
@ghost
Copy link
Author

ghost commented Mar 11, 2022

@IceSentry https://www.getzola.org/documentation/content/syntax-highlighting/#annotations

@bors
Copy link

bors bot commented Mar 11, 2022

Pull request successfully merged into new-book.

Build succeeded:

@bors bors bot changed the title Code validation and format checking [Merged by Bors] - Code validation and format checking Mar 11, 2022
@bors bors bot closed this Mar 11, 2022
bors bot pushed a commit that referenced this pull request Mar 11, 2022
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.

4 participants