📝 Add docs for contributing and private items (#26)

Hello! I took some time between my last PR and this one because I was documenting everything on the code 😍 .

This is the list of contributions, I would like to hear the thoughts of @fdncred and @amtoine about them before merging this PR:

- `docs/CONTRIBUTION.md` added. I explained the why and the how to contribute, and appended a list of general guidelines about the philosophy of the project:
> - Everything should be explained: rust docs, comments, drawings, pick what makes you comfortable, but it is important to make it clear. There will always be some new guy or gal into the project we want to welcome 😄.
> - Use clear variable names and try to avoid confusing abbreviations. Think that your peers may not be fully fluent in english 💬.

- I added a few clippy lints from `clippy:pedantic`, [here](4b97279#diff-b1a35a68f14e696205874893c07fd24fdb88882b47c23cc0e0c80a30c7d53759R6) are them: Clippy will give a warning if one if them is not compliant. 

I added these rules because I would like to improve the quality of upcoming contributions. I'm not sure if this is asking too much from the people who want to be involved in the project, as some of the rules may be a bit annoying. Do you see it doable? Do you think we should delete some or all of the clippy lints?

.github/workflows/main.yml

@@ -30,7 +30,18 @@ jobs:
  - name: Setup Rust toolchain and cache
  uses: actions-rust-lang/[email protected]
  - name: Clippy
- run: cargo clippy --no-deps
+ run: |
+ cargo clippy \
+ --no-deps \
+ -- \
+ -D rustdoc::broken_intra_doc_links \
+ -W missing_docs \
+ -W clippy::missing_docs_in_private_items \
+ -W clippy::explicit_iter_loop \
+ -W clippy::explicit_into_iter_loop \
+ -W clippy::semicolon_if_nothing_returned \
+ -W clippy::doc_markdown \
+ -W clippy::manual_let_else
 
  test:
  name: test rust files

.github/workflows/pull_request.yml

@@ -23,7 +23,18 @@ jobs:
  - name: Setup Rust toolchain and cache
  uses: actions-rust-lang/[email protected]
  - name: Clippy
- run: cargo clippy --no-deps
+ run: |
+ cargo clippy \
+ --no-deps \
+ -- \
+ -D rustdoc::broken_intra_doc_links \
+ -W missing_docs \
+ -W clippy::missing_docs_in_private_items \
+ -W clippy::explicit_iter_loop \
+ -W clippy::explicit_into_iter_loop \
+ -W clippy::semicolon_if_nothing_returned \
+ -W clippy::doc_markdown \
+ -W clippy::manual_let_else
 
  test:
  name: test rust files

README.md

@@ -72,6 +72,4 @@ nufmt my-file1.nu my-file2.nu my-file3.nu
 
 ## Contributing
 
-Submit an issue, or come and say hi in the [Discord](https://discord.gg/NtAbbGn)!
-
-You can mention @AucaCoyan who is active on this repo.
+We have a [contribution guide](docs/CONTRIBUTING.md). If you still have doubts, you can mention @AucaCoyan who is active on this repo.

docs/CONTRIBUTING.md

@@ -0,0 +1,23 @@
+# Contributing to `nufmt`
+
+Salutations! Thanks for coming by and the interest into this project!
+We would like to order the contributions like this:
+
+- Before you start hacking, it is important to **ask first** if your idea or bugfix is in order.
+ Create an issue or come and say hi in the [`#nufmt` channel][nufmt discord channel] by joining [the discord][Nushell discord].
+ We don't bite!.
+
+ It would be sad that you do the effort to clone the project, successfully make the PR, but it wasn't in our plans or there is another PR that is currently adressing that issue.
+
+- After the PR is submitted, the workflows will start to lint, check and test the changes. Please try to stay all green ✅.
+- Sometimes we can take some time to respond. Sorry, we are few and there is much to do here!
+
+## General guidelines and philosophy
+
+This is a list of things we would like to have and mantain across time. Please do your best to abide by.
+
+- Everything should be explained: rust docs, drawings, markdown files, pick what makes you comfortable, but it is important to make it clear. There will always be some new guy or gal into the project we want to welcome 😄.
+- Use clear variable names and try to avoid confusing abbreviations. Think that your peers may not be fully fluent in english 💬.
+
+[Nushell discord]: https://discord.gg/NtAbbGn
+[nufmt discord channel]: https://discord.com/channels/601130461678272522/1117921521520873623

src/config.rs

@@ -1,3 +1,7 @@
+//! Config module
+//!
+//! This keeps all the options, tweaks and dials of the configuration.
+
 use anyhow::Result;
 
 #[derive(Debug)]
@@ -34,6 +38,6 @@ impl Config {
 }
 
 /// Returns a default config.
-pub fn load_config(/* file_path: Option<&Path> */) -> Result<Config> {
+pub fn load(/* file_path: Option<&Path> */) -> Result<Config> {
  Ok(Config::default())
 }

src/formatting.rs

@@ -1,3 +1,8 @@
+//! Formatting module
+//!
+//! In this module occurs most of the magic in `nufmt`.
+//! It has functions to format slice of bytes and some help functions to separate concerns while doing the job.
+//!
 use crate::config::Config;
 use log::trace;
 use nu_parser::{flatten_block, parse, FlatShape};
@@ -7,9 +12,9 @@ use nu_protocol::{
  Span,
 };
 
-// Format an array of bytes
-//
-// Reading the file gives you a list of bytes
+/// Format an array of bytes
+///
+/// Reading the file gives you a list of bytes
 pub fn format_inner(contents: &[u8], _config: &Config) -> Vec<u8> {
  // nice place to measure formatting time
  // let mut timer = Timer::start();
@@ -22,7 +27,7 @@ pub fn format_inner(contents: &[u8], _config: &Config) -> Vec<u8> {
  trace!("parsed block:\n{:?}", &parsed_block);
 
  // check if the block has at least 1 pipeline
- if !block_has_pipilnes(&parsed_block) {
+ if !block_has_pipelines(&parsed_block) {
  trace!("block has no pipelines!");
  println!("File has no code to format.");
  return contents.to_vec();
@@ -41,7 +46,7 @@ pub fn format_inner(contents: &[u8], _config: &Config) -> Vec<u8> {
  let mut start = 0;
  let end_of_file = contents.len();
 
- for (span, shape) in flat.clone().into_iter() {
+ for (span, shape) in flat.clone() {
  // check if span skipped some bytes before the current span
  if span.start > start {
  trace!(
@@ -57,7 +62,7 @@ pub fn format_inner(contents: &[u8], _config: &Config) -> Vec<u8> {
  out.extend(trim_ascii_whitespace(skipped_contents));
  out.push(b'\n');
  } else {
- trace!("The contents doesn't have a '#'. Skipping.")
+ trace!("The contents doesn't have a '#'. Skipping.");
  }
  }
 
@@ -73,14 +78,14 @@ pub fn format_inner(contents: &[u8], _config: &Config) -> Vec<u8> {
  c_bites = trim_ascii_whitespace(c_bites);
  let printable = String::from_utf8_lossy(c_bites).to_string();
  trace!("stripped the whitespace, result: {:?}", printable);
- out.extend(c_bites)
+ out.extend(c_bites);
  }
  FlatShape::Pipe => {
  // here you don't have to strip the whitespace.
  // The pipe is just a pipe `|`.
  //
  // return the pipe AND a space after that
- out.extend("| ".to_string().bytes())
+ out.extend("| ".to_string().bytes());
  }
  FlatShape::External => {
  // External are some key commands
@@ -125,7 +130,7 @@ pub fn format_inner(contents: &[u8], _config: &Config) -> Vec<u8> {
  out.push(b'\n');
  out.extend(trim_ascii_whitespace(remaining_contents));
  } else {
- trace!("The contents doesn't have a '#'. Skipping.")
+ trace!("The contents doesn't have a '#'. Skipping.");
  }
  }
 
@@ -139,6 +144,10 @@ pub fn format_inner(contents: &[u8], _config: &Config) -> Vec<u8> {
  out
 }
 
+/// A wrapper to insert a new line
+///
+/// It is used frequently in `nufmt`, so
+/// we have a wrapper to improve readability of the code.
 fn insert_newline(mut bytes: Vec<u8>) -> Vec<u8> {
  // If I need cfg windows, then I need \r\n
  // let newline = vec![b'\r', b'\n'];
@@ -147,19 +156,32 @@ fn insert_newline(mut bytes: Vec<u8>) -> Vec<u8> {
  bytes
 }
 
+/// Given a slice of bytes, strip all spaces, new lines and tabs found within
+///
+/// Because you don't know how the incoming code is formatted,
+/// the best way to format is to strip all the whitespace
+/// and afterwards include the new lines and indentation correctly
+/// according to the configuration
 pub fn trim_ascii_whitespace(x: &[u8]) -> &[u8] {
- let from = match x.iter().position(|x| !x.is_ascii_whitespace()) {
- Some(i) => i,
- None => return &x[0..0],
- };
+ let Some(from) = x.iter().position(|x| !x.is_ascii_whitespace()) else { return &x[0..0] };
  let to = x.iter().rposition(|x| !x.is_ascii_whitespace()).unwrap();
  &x[from..=to]
 }
 
-fn block_has_pipilnes(block: &Block) -> bool {
+/// Returns true if the Block has at least 1 Pipeline
+///
+/// This function exists because sometimes is passed to `nufmt` an empty String,
+/// or a nu code which the parser can't identify something runnable
+/// (like a list of comments)
+///
+/// We don't want to return a blank file if that is the case,
+/// so this check gives the opportunity to `nufmt`
+/// to know when not to touch the file at all in the implementation.
+fn block_has_pipelines(block: &Block) -> bool {
  !block.pipelines.is_empty()
 }
 
+/// Returns true if the `Span` is the last Span in the slice of `flat`
 fn is_last_span(span: Span, flat: &[(Span, FlatShape)]) -> bool {
  span == flat.last().unwrap().0
 }

src/lib.rs

@@ -13,7 +13,7 @@ use std::path::PathBuf;
 pub mod config;
 pub mod formatting;
 
-/// Reads the file and format it. After that, writes the file inplace
+/// Reads a file and format it. Then writes the file inplace.
 pub fn format_single_file(file: &PathBuf, config: &Config) {
  // read the contents of the file
  let contents = std::fs::read(file)
@@ -24,7 +24,7 @@ pub fn format_single_file(file: &PathBuf, config: &Config) {
 
  // compare the contents
  if formatted_bytes == contents {
- debug!("File is formatted correctly.")
+ debug!("File is formatted correctly.");
  }
 
  // write down the file to path
@@ -33,9 +33,10 @@ pub fn format_single_file(file: &PathBuf, config: &Config) {
  writer
  .write_all(file_bites)
  .expect("something went wrong writing");
- trace!("written")
+ trace!("written");
 }
 
+/// Take a `String` and format it. Then returns a new `String`
 pub fn format_string(input_string: &String, config: &Config) -> String {
  let contents = input_string.as_bytes();
  let formatted_bytes = format_inner(contents, config);

src/main.rs

@@ -36,11 +36,6 @@
 //!
 //! - `-v` or `--version` prints the version and exit
 
-// throw error if finds a broken link in doc
-#![deny(rustdoc::broken_intra_doc_links)]
-// or docs are missing for public members
-#![warn(missing_docs)]
-
 use anyhow::{Ok, Result};
 use clap::Parser;
 use log::trace;
@@ -50,24 +45,34 @@ use std::error::Error;
 use std::io::Write;
 use std::path::PathBuf;
 
+/// wrapper to the successful exit code
 const SUCCESSFUL_EXIT: i32 = 0;
+/// wrapper to the failure exit code
 const FAILED_EXIT: i32 = 1;
 
+/// Main CLI struct.
+///
+/// The derive Clippy API starts from defining the CLI struct
 #[derive(Parser)]
 #[command(author, version, about)]
 struct Cli {
+ /// The list of files passed in the cmdline
+ /// It is required and it cannot be used with `--stdin`
  #[arg(
  required_unless_present("stdin"),
  help = "The file or files you want to format in nu"
  )]
  files: Vec<PathBuf>,
+ /// The string you pass in stdin. You can pass only one string.
  #[arg(
  short,
  long,
  conflicts_with = "files",
  help = "Format the code passed in stdin as a string."
  )]
  stdin: Option<String>,
+ /// The optional config file you can pass in the cmdline
+ /// You can only pass a file config, not a flag config
  #[arg(short, long, help = "The configuration file")]
  config: Option<PathBuf>,
 }
@@ -123,7 +128,7 @@ fn execute_string(string: Option<String>, options: &Config) -> Result<i32> {
 /// Sends the files to format in lib.rs
 fn execute_files(files: Vec<PathBuf>, options: &Config) -> Result<i32> {
  // walk the files in the vec of files
- for file in files.iter() {
+ for file in &files {
  if !file.exists() {
  eprintln!("Error: {} not found!", file.to_str().unwrap());
  return Ok(FAILED_EXIT);
@@ -133,11 +138,10 @@ fn execute_files(files: Vec<PathBuf>, options: &Config) -> Result<i32> {
  file.to_str().unwrap()
  );
  return Ok(FAILED_EXIT);
- } else {
- // send the file to lib.rs
- println!("formatting file: {:?}", file);
- format_single_file(file, options);
  }
+ // send the file to lib.rs
+ println!("formatting file: {:?}", file);
+ format_single_file(file, options);
  }
 
  Ok(SUCCESSFUL_EXIT)

toolkit.nu

@@ -0,0 +1,76 @@
+# this module regroups a bunch of development tools to make the development
+# process easier for anyone.
+#
+# the main purpose of `toolkit` is to offer an easy to use interface for the
+# developer during a PR cycle, namely to (**1**) format the source base,
+# (**2**) catch classical flaws in the new changes with *clippy* and (**3**)
+# make sure all the tests pass.
+
+# print the pipe input inside backticks, dimmed and italic, as a pretty command
+def pretty-print-command [] {
+ $"`(ansi default_dimmed)(ansi default_italic)($in)(ansi reset)`"
+}
+
+# check standard code formatting and apply the changes
+export def fmt [
+ --check: bool # do not apply the format changes, only check the syntax
+ --verbose: bool # print extra information about the command's progress
+] {
+ if $verbose {
+ print $"running ('toolkit fmt' | pretty-print-command)"
+ }
+
+ if $check {
+ try {
+ cargo fmt --all -- --check
+ } catch {
+ error make --unspanned {
+ msg: $"\nplease run ('toolkit fmt' | pretty-print-command) to fix formatting!"
+ }
+ }
+ } else {
+ cargo fmt --all
+ }
+}
+
+# check that you're using the standard code style
+#
+# > it is important to make `clippy` happy :relieved:
+export def clippy [
+ --verbose: bool # print extra information about the command's progress
+] {
+ if $verbose {
+ print $"running ('toolkit clippy' | pretty-print-command)"
+ }
+
+ try {(
+ cargo clippy
+ --workspace
+ --
+ -D rustdoc::broken_intra_doc_links
+ -W missing_docs
+ -W clippy::missing_docs_in_private_items
+ -W clippy::explicit_iter_loop
+ -W clippy::explicit_into_iter_loop
+ -W clippy::semicolon_if_nothing_returned
+ -W clippy::doc_markdown
+ -W clippy::manual_let_else
+ )} catch {
+ error make --unspanned {
+ msg: $"\nplease fix the above ('clippy' | pretty-print-command) errors before continuing!"
+ }
+ }
+}
+
+# check that all the tests pass
+export def test [
+ --fast: bool # use the "nextext" `cargo` subcommand to speed up the tests (see [`cargo-nextest`](https://nexte.st/) and [`nextest-rs/nextest`](https://github.com/nextest-rs/nextest))
+] {
+ if $fast {
+ cargo nextest run --all
+ } else {
+ cargo test --workspace
+ }
+}
+
+export def main [] { help toolkit }