Fix makers formatting #535
Conversation
src/lib/mod.rs
Outdated
| @@ -195,5 +195,9 @@ mod version; | |||
|
|
|||
| /// Handles the command line arguments and executes the runner. | |||
| pub fn run_cli(command_name: String, sub_command: bool) { | |||
| #[cfg(windows)] | |||
| if let Err(err) = ansi_term::enable_ansi_support() { | |||
| eprintln!("error enabling ANSI support: {:?}", err); | |||
There was a problem hiding this comment.
not sure we need to print it to the users.... if it doesnt work, no big deal i guess. wdyt?
also, why not do it in makers.rs? since cargo is already fixing it for us, we can just push the fix to the makers executable only.
There was a problem hiding this comment.
I wrote it in makers.rs first but then I've moved it to CLI to satisfy linters (otherwise it breaks the rule not used crate ansi_term or something like that).
I've added eprintln for easier user reporting when it fails.
So
I'll move it back to makers.rs and satisfy/disable the linter rule for it somehow.
Should I remove that eprintln call?
There was a problem hiding this comment.
i wonder how to silence that linter for specific case, so in future i don't forget some crates in...
maybe than, just add some bool from makers/main -> cli that tell it if to use it or not... crappy hack.
eprintln -> ya, lets remove it. i'm not sure users would find that helpful and many people already complain that cargo make prints out too much stuff.
There was a problem hiding this comment.
Code updated in the latest commit.
From https://doc.rust-lang.org/rustc/lints/listing/allowed-by-default.html#unused-crate-dependencies:
This lint is "allow" by default because it can provide false positives depending on how the build system is configured. For example, when using Cargo, a "package" consists of multiple crates (such as a library and a binary), but the dependencies are defined for the package as a whole. If there is a dependency that is only used in the binary, but not the library, then the lint will be incorrectly issued in the library.
|
@MartinKavik thanks a lot!!!
what does this mean? i'm running ci flow on windows today, so what was i missing?
can you explain this one? why do we need powershell for cargo make? i usually default to cmd. |
It's pretty "standard error" for many tools on Windows. The first related issue I was able to find: rust-lang/rustfmt#1873. It happens at least in Git Bash and PowerShell. Maybe it will be fixed in the next
One test:
You can switch between |
MartinKavik
force-pushed
the
fix/makers_formatting
branch
from
ffe4433
to
419f3c5
Compare
Codecov Report
@@ Coverage Diff @@
## master #535 +/- ##
===========================================
- Coverage 92.92% 65.33% -27.60%
===========================================
Files 98 98
Lines 18672 18672
===========================================
- Hits 17351 12199 -5152
- Misses 1321 6473 +5152
Continue to review full report at Codecov.
|
| @@ -21,6 +21,8 @@ fn get_name() -> String { | |||
| } | |||
|
|
|||
| fn main() { | |||
| #[cfg(windows)] | |||
| let _ = ansi_term::enable_ansi_support(); | |||
There was a problem hiding this comment.
question, do we need the let _ here?
There was a problem hiding this comment.
Alternative would be something like:
#[allow(unused_must_use)]
{
#[cfg(windows)]
ansi_term::enable_ansi_support();
}
I didn't test it but I've found this workaround on stackoverflow or a Rust forum / issue.
This doesn't work for some reasons:
#[allow(unused_must_use)]
#[cfg(windows)]
ansi_term::enable_ansi_support();There was a problem hiding this comment.
lets leave it. thanks for the explanation.
|
@MartinKavik thanks a lot for the answers :)
Will probably merge it later today/tomorrow. |
|
Ran few manual tests on linux. looks awesome. thanks again @MartinKavik |
Fixes #533
There are two commits, the first fixes the issue, the second one fixes
cargo make ci-flowon Windows.The reason why formatting works when you run
makersindirectly (throughcargo runorcargo make) is thatcargouse a special print function: https://github.com/rust-lang/cargo/blob/e4aebf0a039ca3787f3ce98a9d469a3187f83706/src/cargo/core/shell.rs#L327-L340 with Windows-only code. Note: There are multiple Unix/Windows "hacks" in the linked file to make the shell work as expected.Another way is to enable ANSI support globally, just like Trunk or other apps do it. This approach has been used in this PR. We need to call a native API to enable it: https://github.com/ogham/rust-ansi-term/blob/ff7eba98d55ad609c7fcc8c7bb0859b37c7545cc/src/windows.rs