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: use cobra library instead of flag #10

Merged
merged 3 commits into from
Sep 27, 2024
Merged

Conversation

d3cryptofc
Copy link
Contributor

$ ./hyprnotify --help

DBus Implementation of Freedesktop Notification spec for 'hyprctl notify'

Usage:
  hyprnotify [flags]

Flags:
  -h, --help       help for hyprnotify
  -s, --no-sound   disable sound, silent mode

I'm suggesting this PR for the following reasons:

  • It has 'shorthand', a short version of the command, resulting in a clearer manual to read and avoiding the need to create several flags as aliases.
  • In parallel I'm working on adding a flag called --font-size to define a fixed size for the font, and this library would make reading much more pleasant. (In fact, I already have the code ready and working, but I still need to organize the commits)

I know it's a boring situation but I ask you to think about it, I think it's worth it. Thank you!

This library will replace the previous `flag` library, making it more practical to use, including when creating aliases.
See more details on previous commit.
@codelif
Copy link
Owner

codelif commented Sep 25, 2024

dude you are on fire 🔥

@d3cryptofc
Copy link
Contributor Author

dude you are on fire 🔥

I'm tired of Mako and Dunst, when I found this project I almost died of happiness haha

@codelif
Copy link
Owner

codelif commented Sep 25, 2024

I know it's a boring situation but I ask you to think about it, I think it's worth it. Thank you!

No it's not a boring situation. Infact it might make #6 easier to implement.

@codelif
Copy link
Owner

codelif commented Sep 25, 2024

* In parallel I'm working on adding a flag called `--font-size` to define a fixed size for the font, and this library would make reading much more pleasant. (In fact, I already have the code ready and working, but I still need to organize the commits)

gosh, hats off to the 10x dev. Made my day.

@d3cryptofc
Copy link
Contributor Author

Is there anything missing from this PR?

@codelif
Copy link
Owner

codelif commented Sep 25, 2024

see the review,

also the automatic builds are failing for some reason I am too tired to look into it because it's 3:30 AM rn. Will look into it tomorrow (as in the tomorrow I will wake up into)

@d3cryptofc
Copy link
Contributor Author

OK. By the way, sorry, I forgot about the time zone, here in my country it has just fallen dusk.


internal.InitDBus(enableSound)
CmdFlags.BoolVarP(&enableSound, "no-sound", "s", false, "disable sound, silent mode")
Copy link
Owner

Choose a reason for hiding this comment

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

this reverses the enableSound condition, so setting the --no-sound flag actually enables sound and vice versa

Copy link
Contributor Author

@d3cryptofc d3cryptofc Sep 26, 2024

Choose a reason for hiding this comment

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

I just copied the original, how strange is that..

https://github.com/codelif/hyprnotify/blob/main/cmd/hyprnotify/main.go#L12-L14

and I thought there was no sound due to some lack of dependence 🤔

Edit 2:

It seems that in version 7.0 it was correct:

https://github.com/codelif/hyprnotify/blob/v0.7.0/cmd/hyprnotify/main.go#L9-L16

But in the future it was reversed in this commit:

2c34d12

I will make the correction commit.

@codelif
Copy link
Owner

codelif commented Sep 26, 2024

btw I fixed the failing builds, it was using an older version of a action. teehee

@codelif codelif merged commit 91de923 into codelif:main Sep 27, 2024
1 check passed
@codelif
Copy link
Owner

codelif commented Sep 27, 2024

LGTM! Kudos to you.

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.

2 participants