Closed amtoine closed 1 year ago
cc/ @AucaCoyan
i think these errors are too strong and Rust is expressive enough in these cases :thinking: 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 :relieved:
Note the patch would have to be adapted for the CI
in d4bb23a, i've implemented the windows case for adding a newline, which adds a \r\n
but this breaks all the tests :thinking:
there are a bunch of pub
lic 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
?
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! 🎉
Hello! Thanks for the time sinked here.
ehe no worries, that was fun :yum:
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 a43cc16I'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 :sweat_smile: 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 :laughing: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 :+1: 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 :relieved: the rests are either marked with- nothing to make them private
pub(crate)
to make them public in the scope of thenu_formatter
crate only :smirk:thought_balloon Thanks again for the refactor and suggestions! tada
i'm glad you like them :relieved:
maybe just a last :+1: with the last commits and then let's land this :partying_face:
i've alread seen PRs going out control with that 😅
LOL
Merge it away! I'm happy with these changes
LOL
:laughing:
Merge it away! I'm happy with these changes
cooool, thanks for the review, that was fun :triumph:
this PR is all about refactoring the formatting library and executable :yum:
details
nu_formatter
nufmt
print
in favor oflog
cli.files
,#[cfg(windows)]
and exit codes)nu_formatter
public