Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: mise hints #2479

Merged
merged 1 commit into from
Sep 3, 2024
Merged

feat: mise hints #2479

merged 1 commit into from
Sep 3, 2024

Conversation

roele
Copy link
Contributor

@roele roele commented Aug 19, 2024

First draft for a very pragmatic approach on mise hints. Just adds a simple hint! macro with static args which respects the setting. Since i wasn't able to figure out yet how to have a multi-type setting i thought it might be handy to use disable_hints= ["*"] as a way to disable all commands (IIRC we use * for running all tasks as well?)

It might be nicer to have something which we could pin to the run method such as #[hint(...)] (derive, proc_macro?). Not sure if Rust has something similar to Aspects in Java which allows you to run stuff after/before method calls etc.

Still a bit undecided about the styling. We probably should stick to a dim prefix like info messages but should highlight the command somehow with bold?

Screenshot 2024-08-20 at 00 25 26

Fixes #2460

src/cli/install.rs Outdated Show resolved Hide resolved
@jdx
Copy link
Owner

jdx commented Aug 19, 2024

I like the *!

@roele roele force-pushed the issues/2460 branch 7 times, most recently from 0680219 to b9b8b95 Compare August 26, 2024 07:43
@jdx
Copy link
Owner

jdx commented Aug 29, 2024

I think a great addition to this would be the current warning about precompiled python binaries which is probably annoying to people that already are aware of its limitations

@@ -31,6 +31,14 @@
"description": "disables built-in shorthands",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can actually probably just remove this file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was already wondering if this is still useful since all settings can also be part of .mise.toml

@roele roele force-pushed the issues/2460 branch 2 times, most recently from 7417675 to 2793296 Compare August 30, 2024 18:21
@roele
Copy link
Contributor Author

roele commented Aug 30, 2024

@jdx tried to disable the hints for the tests but one test keeps failing and i can't figure out why. it passes in my IDE and when run via cargo test but not via nextest.

--- TRY 3 STDOUT:        mise::bin/mise cli::install::tests::test_install_nothing ---

running 1 test
test cli::install::tests::test_install_nothing ... ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot file: src/cli/snapshots/mise__cli__install__tests__install_nothing.snap
Snapshot: install_nothing
Source: src/cli/install.rs:193
────────────────────────────────────────────────────────────────────────────────
Expression: output
────────────────────────────────────────────────────────────────────────────────
+new results
────────────┬───────────────────────────────────────────────────────────────────
          0 │+mise hint install multiple versions simultaneously with mise install python@3.8 python@3.9
────────────┴───────────────────────────────────────────────────────────────────
To update snapshots run `cargo insta review`
Stopped on the first failure. Run `cargo insta test` to run all snapshots.
FAILED

failures:

failures:
    cli::install::tests::test_install_nothing

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 233 filtered out; finished in 0.12s

@roele
Copy link
Contributor Author

roele commented Aug 30, 2024

Is it just me or does this feel a bit annoying and intrusive? How would we educate users about disabling the hints?
Maybe something interactive (confirmation/dialog) which disables it after confirmation would be easier to use and more tolerable? Or maybe i am overthinking this again and we should start simple...

@roele roele force-pushed the issues/2460 branch 3 times, most recently from e00e9e5 to 21321f7 Compare August 30, 2024 20:32
warn!("if you experience issues with this python (e.g.: running poetry), switch to python-build");
warn!("by running: mise settings set python_compile 1");
hint!(
"install",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if we need more fine grained control here

@roele roele force-pushed the issues/2460 branch 3 times, most recently from 2432fc2 to 922b600 Compare September 1, 2024 10:19
@roele roele marked this pull request as ready for review September 1, 2024 10:28
@jdx
Copy link
Owner

jdx commented Sep 1, 2024

yeah I think we should show a second line like: mise hint disable this thing with "mise settings set ..."

@roele
Copy link
Contributor Author

roele commented Sep 2, 2024

it's a bit unfortunate that array settings are not additive, so we need to include existing settings mise settings set disable_hints install,plugins otherwise it would overwrite a disable_hints = ["plugins"] instead adding to it.

@jdx
Copy link
Owner

jdx commented Sep 3, 2024

yeah maybe at some point we could add a mise settings add disable_hints or something

@jdx jdx merged commit 595c70b into jdx:main Sep 3, 2024
10 checks passed
@roele roele deleted the issues/2460 branch September 3, 2024 16:09
triarius pushed a commit to triarius/mise that referenced this pull request Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mise hints
2 participants