refactor and polish the source code of the library #29

amtoine · 2023-06-15T18:13:10Z

this PR is all about refactoring the formatting library and executable 😋

details

Note
i've made this PR such that it's easy to review commit by commit and there are explaination in all the commit bodies 👌

rename the library to nu_formatter
link to the README in the doc of nufmt
make Clippy run on all the targets, e.g. including benchmarks
remove (almost) all print in favor of log
fix some "hacks" in the code (cli.files, #[cfg(windows)] and exit codes)
simplify the benchmarks
rename some functions
remove useless comments and simplify the documentation when possible
relax Clippy on missing documentation
do not make all items of nu_formatter public

the goal of this commit is to make a clear distinction between the library which does all the heavy lifting and the binary application which mainly just parses the arguments and calls the library. because i think the application is clearly called `nufmt` i propose here to change the name of the library to `nu_formatter`. i also thought about `nufmt_lib`! this has the huge benefit of making two separate items in the `cargo doc` instead of two with the same `nufmt` name :/ ``` cargo doc --document-private-items --bin nufmt --open ``` directly opens the doc on the application page with the help and the usage, which i find bette :)

this commit is here to make the maintainance of the doc easier: change the README and bam, the doc of the `nufmt` binary is up-to-date!

otherwise, warnings from the documentation and the benchmarks for instance are not seen when running the CI or `toolkit clippy` :o we also make sure we use the same clippy command in the toolkit.

when formatting a string from stdin we need to return the result directly to stdout.

in order to have a slice from `cli.files`, i think `cli.files[..]`, i.e. the whole slice from `cli.files`, is easier to work with than `&*cli.files` and does not require any additional comment.

because on windows we add `\r\n` and only `\n` on linux, we can just conditionally add `\r` when on windows and then always add `\n`.

this commit also removes the `Result` and `Ok` because there is no such error at all in `nu_formatter` for now.

amtoine · 2023-06-16T08:01:32Z

cc/ @AucaCoyan

some questions remaining

Clippy missing doc errors (see here)

i think these errors are too strong and Rust is expressive enough in these cases 🤔
i propose applying the following patch to the toolkit

diff --git a/toolkit.nu b/toolkit.nu
index 5b421e8..6e986f0 100644
--- a/toolkit.nu
+++ b/toolkit.nu
@@ -51,8 +51,6 @@ export def clippy [
         --
             -D warnings
             -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

to let developer add documentation when it makes the most sense 😌

Note
the patch would have to be adapted for the CI

failing test on windows (see here)

in d4bb23a, i've implemented the windows case for adding a newline, which adds a \r\n but this breaks all the tests 🤔

public functions

there are a bunch of public functions that are exported outside of nu_formatter

> rg '^\s*pub' | lines | parse "{file}:{match}"
╭────┬───────────────────┬──────────────────────────────────────────────────────────────────────────────╮
│  # │       file        │                                    match                                     │
├────┼───────────────────┼──────────────────────────────────────────────────────────────────────────────┤
│  0 │ src/formatting.rs │ pub fn format_inner(contents: &[u8], _config: &Config) -> Vec<u8> {          │
│  1 │ src/formatting.rs │ pub fn trim_ascii_whitespace(x: &[u8]) -> &[u8] {                            │
│  2 │ src/lib.rs        │ pub mod config;                                                              │
│  3 │ src/lib.rs        │ pub mod formatting;                                                          │
│  4 │ src/lib.rs        │ pub fn format_single_file(file: &PathBuf, config: &Config) {                 │
│  5 │ src/lib.rs        │ pub fn format_string(input_string: &String, config: &Config) -> String {     │
│  6 │ src/config.rs     │ pub struct Config {                                                          │
│  7 │ src/config.rs     │     pub tab_spaces: usize,                                                   │
│  8 │ src/config.rs     │     pub max_width: usize,                                                    │
│  9 │ src/config.rs     │     pub margin: usize,                                                       │
│ 10 │ src/config.rs     │     pub fn new(tab_spaces: usize, max_width: usize, margin: usize) -> Self { │
╰────┴───────────────────┴──────────────────────────────────────────────────────────────────────────────╯

i'm just wondering if we need that many exported things, e.g. we could use pub(crate) for those required inside the library and pub for those needed in nufmt?

AucaCoyan · 2023-06-16T09:51:34Z

Hello! Thanks for the time sinked here.

Clippy missing doc errors

Yeah, clippy can be very annoying sometimes. I'm ok with removing those 2 lints. Especially with the private items.

failing test on windows

I'm not sure what is making the test fail, but before \r\n it was passing both in windows and linux. I believe the implementation of \n in low code rust makes the correct assumption whether is \n or \r\n.
To add one more layer to the onion, we usually work on linux EOL when we work on git. So if the formatter edits code files, I don't see the problem of using \n.
Should we format \n for all files in all OSs?

public functions

You are correct. That exports were on my TODO list.
I didn't have time (and patience) to finish the rust book yet, so sometimes I go for the simpler way, not because it's actually better, but because I'm not confident on the meaning of the code 😜. I'm so glad you could resolve that!

💭 Thanks again for the refactor and suggestions! 🎉

- `trim_ascii_whitespaces` is never used outside `formatting` - the `formatting` commands are only used internally

amtoine · 2023-06-16T10:44:10Z

Hello! Thanks for the time sinked here.

ehe no worries, that was fun 😋

Yeah, clippy can be very annoying sometimes. I'm ok with removing those 2 lints. Especially with the private items.

i've disabled the two annoying Clippy items in 13b91dc and made -D warnings explicit in a43cc16

I'm not sure what is making the test fail, but before \r\n it was passing both in windows and linux. I believe the implementation of \n in low code rust makes the correct assumption whether is \n or \r\n. To add one more layer to the onion, we usually work on linux EOL when we work on git. So if the formatter edits code files, I don't see the problem of using \n. Should we format \n for all files in all OSs?

i've reverted d4bb23a in 3433524, i think \r\n is not a good idea, i've alread seen PRs going out control with that 😅
where like the global changes are just a block of red and a block of green on wholes files and the actual changes are like 1 line in there 😆

You are correct. That exports were on my TODO list. I didn't have time (and patience) to finish the rust book yet, so sometimes I go for the simpler way, not because it's actually better, but because I'm not confident on the meaning of the code stuck_out_tongue_winking_eye. I'm so glad you could resolve that!

i fixed the visibility in fd032a0 👍
if you cargo doc --no-deps --lib --open, you'll publicly see

config
format_single_file
format_string
which are, i believe, the only one that needs to be publicly available, for now of course 😌
the rests are either marked with
nothing to make them private
pub(crate) to make them public in the scope of the nu_formatter crate only 😏

thought_balloon Thanks again for the refactor and suggestions! tada

i'm glad you like them 😌

maybe just a last 👍 with the last commits and then let's land this 🥳

AucaCoyan · 2023-06-16T11:00:53Z

i've alread seen PRs going out control with that 😅

LOL

Merge it away! I'm happy with these changes

amtoine · 2023-06-16T11:09:57Z

LOL

😆

Merge it away! I'm happy with these changes

cooool, thanks for the review, that was fun 😤

amtoine added 10 commits June 16, 2023 08:42

link the main.rs doc to the README

d172e01

this commit is here to make the maintainance of the doc easier: change the README and bam, the doc of the `nufmt` binary is up-to-date!

run *Clippy* on --all-targets

e662352

otherwise, warnings from the documentation and the benchmarks for instance are not seen when running the CI or `toolkit clippy` :o we also make sure we use the same clippy command in the toolkit.

use log instead of print except with formatting a string

e8dd93a

when formatting a string from stdin we need to return the result directly to stdout.

fix the cli.files hack

5b5f117

in order to have a slice from `cli.files`, i think `cli.files[..]`, i.e. the whole slice from `cli.files`, is easier to work with than `&*cli.files` and does not require any additional comment.

implement the "windows newline" trick

d4bb23a

because on windows we add `\r\n` and only `\n` on linux, we can just conditionally add `\r` when on windows and then always add `\n`.

use a more expressive exit_code enum

eae1b46

this commit also removes the `Result` and `Ok` because there is no such error at all in `nu_formatter` for now.

simplify the benchmark

0dbb734

rename execute_string and execute_format in main

880ecc1

remove useless comments and document

98324ec

amtoine force-pushed the refactor/polish-the-source-code-of-the-library branch from 2954fbf to 98324ec Compare June 16, 2023 07:18

make *Clippy* happy except for missing doc

c183e04

amtoine added 4 commits June 16, 2023 12:24

remove the Clippy errors for missing documentation

13b91dc

do not add a \r when on windows, always a single \n

3433524

fix the visibility of nu_formatter items

fd032a0

- `trim_ascii_whitespaces` is never used outside `formatting` - the `formatting` commands are only used internally

be explicit about -D warnings in the CI

a43cc16

amtoine marked this pull request as ready for review June 16, 2023 10:38

amtoine merged commit acd75e7 into nushell:main Jun 16, 2023
4 checks passed

amtoine deleted the refactor/polish-the-source-code-of-the-library branch June 16, 2023 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor and polish the source code of the library #29

refactor and polish the source code of the library #29

amtoine commented Jun 15, 2023 •

edited

amtoine commented Jun 16, 2023 •

edited

AucaCoyan commented Jun 16, 2023

amtoine commented Jun 16, 2023

AucaCoyan commented Jun 16, 2023

amtoine commented Jun 16, 2023

refactor and polish the source code of the library #29

refactor and polish the source code of the library #29

Conversation

amtoine commented Jun 15, 2023 • edited

details

amtoine commented Jun 16, 2023 • edited

some questions remaining

Clippy missing doc errors (see here)

failing test on windows (see here)

public functions

AucaCoyan commented Jun 16, 2023

amtoine commented Jun 16, 2023

AucaCoyan commented Jun 16, 2023

amtoine commented Jun 16, 2023

amtoine commented Jun 15, 2023 •

edited

amtoine commented Jun 16, 2023 •

edited