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(cmd_duration): make notify feature optional (compat with nix darwin) #3855

Merged
merged 1 commit into from
Apr 12, 2022

Conversation

happysalada
Copy link
Contributor

Description

the notify-rust has some very cryptic compilation error on nix and darwin. (reference issue NixOS/nixpkgs#160876)

In the meanwhile, version has been moved forward in nix to not penalize linux users.

Hopefully we can find the source of the error, but in the meanwhile, making this dependency optional is very helpful!

Thank you again for your terminal! (going a month without it, feels like going back to the stone age #addicted)

Motivation and Context

Closes #

Screenshots (if appropriate):

How Has This Been Tested?

I ran cargo test, cargo test --no-default-features, cargo clippy --all-features --all-targets and couldn't find problems with it, let me know however if you need changes.

Checklist:

  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

@happysalada
Copy link
Contributor Author

Semantic pull request is unhappy, is this a feat or a fix ?
I couldn't decide.

@davidkna
Copy link
Member

I would consider this a feat. Please don't disable the whole cmd_duration module if the feature is disabled. Maybe try building a partial revert for #3547? Though I would prefer if you keep using notify instead of notify-rust as the feature name.

@davidkna davidkna changed the title notify: make feature optional (compat with nix darwin) feat(cmd_duration): make notify feature optional (compat with nix darwin) Apr 10, 2022
@happysalada
Copy link
Contributor Author

I've only made the calls to the notification optional.
I could have made the call to undistract_me optional, but this would require a cfg_if! so I thought this was simpler.
Let me know if anything.

@happysalada
Copy link
Contributor Author

alright, I think I've got it this time.
Compilation with no warnings.
I've also changed the commit message to reflect the title of this PR
let me know if anything.

@happysalada
Copy link
Contributor Author

my bad for forgetting those, fixed and pushed.

@davidkna davidkna merged commit efaab49 into starship:master Apr 12, 2022
@davidkna
Copy link
Member

Thanks for the contribution @happysalada!

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.

None yet

2 participants