tldr-pages / tlrc

A tldr client written in Rust
http://tldr.sh/tlrc/
MIT License
232 stars 9 forks source link

To fix the errors and improve the code, I would suggest these chunks : ) #62

Closed Aditya-dom closed 4 months ago

Aditya-dom commented 4 months ago

fix the errors

What errors?

I'm sorry, but I see no benefit in what you've changed. use statements are there to make the code more readable, instead of using long, full paths to every type and function. Some of what you changed isn't even valid Rust, for example returning an i32 from main. Tests & Clippy are failing because the program doesn't compile now.

Please update the code to at least make it compile and pass CI checks.

In Rust, the default integer type is i32 because it is generally the fastest, even on 64-bit systems.

Yes, I agree with your passing CI checks.

acuteenvy commented 4 months ago

In response to your message on Matrix:

I have submitted the pull request, and I understand your concerns. However, I would like to know why the tests and Clippy are failing. Additionally, could you provide guidance on how to successfully compile the program?

The code from the main branch of this repository compiles successfully. All you have to do is run cargo build for the debug build, or cargo build --release for the optimized build.

The tests and Clippy are failing, because your code does not compile. There are a couple reasons for that. Previously, you've changed the return type of main to i32, which does not implement the Termination trait. Please see the CI logs for more information - the compiler tells you exactly what is wrong. You can also check if the CI will fail before creating a PR - just try to compile and test the program, make sure cargo clippy doesn't output any warnings and format your code with rustfmt.

The current version from your fork does not compile successfully, because you're trying to call a non-const function in a static declaration:

error[E0015]: cannot call non-const fn `<AtomicBool as std::default::Default>::default` in statics
  --> src/main.rs:22:47
   |
22 | static QUIET: std::sync::atomic::AtomicBool = std::sync::atomic::AtomicBool::default();
   |                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: calls in statics are limited to constant functions, tuple structs and tuple variants
   = note: consider wrapping this expression in `Lazy::new(|| ...)` from the `once_cell` crate: https://crates.io/crates/once_cell

For more information about this error, try `rustc --explain E0015`.

The fact that it's not allowed aside, may I ask, why are you even changing this? Would

static QUIET: std::sync::atomic::AtomicBool = std::sync::atomic::AtomicBool::default();

really be more readable to you than

use std::sync::atomic::AtomicBool;

static QUIET: AtomicBool = AtomicBool::new(false);

? What's the advantage of using full paths everywhere?

Aditya-dom commented 4 months ago

In response to your message on Matrix:

I have submitted the pull request, and I understand your concerns. However, I would like to know why the tests and Clippy are failing. Additionally, could you provide guidance on how to successfully compile the program?

The code from the main branch of this repository compiles successfully. All you have to do is run cargo build for the debug build, or cargo build --release for the optimized build.

The tests and Clippy are failing, because your code does not compile. There are a couple reasons for that. Previously, you've changed the return type of main to i32, which does not implement the Termination trait. Please see the CI logs for more information - the compiler tells you exactly what is wrong. You can also check if the CI will fail before creating a PR - just try to compile and test the program, make sure cargo clippy doesn't output any warnings and format your code with rustfmt.

The current version from your fork does not compile successfully, because you're trying to call a non-const function in a static declaration:

error[E0015]: cannot call non-const fn `<AtomicBool as std::default::Default>::default` in statics
  --> src/main.rs:22:47
   |
22 | static QUIET: std::sync::atomic::AtomicBool = std::sync::atomic::AtomicBool::default();
   |                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: calls in statics are limited to constant functions, tuple structs and tuple variants
   = note: consider wrapping this expression in `Lazy::new(|| ...)` from the `once_cell` crate: https://crates.io/crates/once_cell

For more information about this error, try `rustc --explain E0015`.

The fact that it's not allowed aside, may I ask, why are you even changing this? Would

static QUIET: std::sync::atomic::AtomicBool = std::sync::atomic::AtomicBool::default();

really be more readable to you than

use std::sync::atomic::AtomicBool;

static QUIET: AtomicBool = AtomicBool::new(false);

? What's the advantage of using full paths everywhere?

Thanks for explaining, and I've understood your point. This is more readable 👍🏻

use std::sync::atomic::AtomicBool;

static QUIET: AtomicBool = AtomicBool::new(false);

I am more familiar with this way of representing it as a bool.

static QUIET: std::sync::atomic::AtomicBool = std::sync::atomic::AtomicBool::default();

No harm done; you're right, as always. Once again, thanks for taking the time to explain things to me.