waycrate / swhkd

Sxhkd clone for Wayland (works on TTY and X11 too)
https://git.sr.ht/~shinyzenith/swhkd
BSD 2-Clause "Simplified" License
667 stars 47 forks source link

[feat] use clap Derive API to accept command line arguments #247

Closed InnocentZero closed 3 months ago

InnocentZero commented 4 months ago

This patch removes the usage of clap::Command and arg macros, and bumps the library version to use the new derive API which is the recommended method upstream.

Shinyzenith commented 4 months ago

Hi, due to health reasons I had to take a small break from my responsibilities. CC: @zubairmh Can someone just test this pr? This seems good to go for me.

I'll merge if it's fine.

InnocentZero commented 4 months ago

@zubairmh Any updates on this? I tested this from my end and everything looked fine to me.

newtoallofthis123 commented 4 months ago

Hey @InnocentZero! I just tried testing out the PR and it seems that the config struct has some small errors that cause the daemon to panic.

The exact error is in the comment I left if that helps.

The arg parser tried to make an option from the first char in the option, so in this case, in both cases of config (-c) / cooldown(-c) as well as debug(-d) / devices(-d), it sort of conflicts.

Removing the short would most likely fix this problem. Other than that, everything else looks awesome :)

InnocentZero commented 4 months ago

Oh that was my bad. I did not thoroughly check the daemon and committed the changes. Should have paid attention! Thanks for pointing it out @newtoallofthis123 !

InnocentZero commented 4 months ago

@Shinyzenith alternatively we can also use Cooldown and Devices for having short argument format for everything. Please choose what you'd like.

Shinyzenith commented 4 months ago

Devices

CC: @newtoallofthis123 How do you feel about this?

newtoallofthis123 commented 4 months ago

Usually bool arguments have a short option 😄 , so the what @InnocentZero has already is quite cool. Moreover, -d for debug is a bit more pragmatic and so is -c for CoolDown.

I did just test the latest commit, everything seems to work fine now :)

PS: I think you might've accidentally pushed the trim_start_matches commit as well. Might be better to revert it back.

InnocentZero commented 4 months ago

I did just test the latest commit, everything seems to work fine now :)

Glad to have it sorted, and sorry for the mixup :sweat_smile:

PS: I think you might've accidentally pushed the trim_start_matches commit as well. Might be better to revert it back.

Oh I just rebased my local branch based on upstream to prevent merge conflicts. I've revised the commits.

InnocentZero commented 3 months ago

@Shinyzenith can you go through this once? Thanks.

Shinyzenith commented 3 months ago

Hi -c got changed to -C from what I can tell. This is an undesired breaking change.

Shinyzenith commented 3 months ago

Same goes for -d correct me if I am mistaken.

InnocentZero commented 3 months ago

Hi -c got changed to -C from what I can tell. This is an undesired breaking change.

This is for config option. The former was -C for cooldown and -c for config. However what I implemented was the feedback given by @newtoallofthis123.

As for device and debug, their original short options are retained afaik.

I'll change it to the previous defaults.

Shinyzenith commented 3 months ago

Hi -c got changed to -C from what I can tell. This is an undesired breaking change.

This is for config option. The former was -C for cooldown and -c for config. However what I implemented was the feedback given by @newtoallofthis123.

Hi, Apologies forn the misunderstanding. I am a maintainer so it should've gone under a review cycle, but it's fine now that it's reverted.😊

newtoallofthis123 commented 3 months ago

Sorry, I hadn't noticed the flags being used currently 😅

Shinyzenith commented 3 months ago

Ready to merge this, can you just rebase it onto the current main to resolve the conflicts? If not, I'll do it myself

Shinyzenith commented 3 months ago

Hi conflict still exists and now the commits are repeated.

InnocentZero commented 3 months ago

@Shinyzenith can you check now, I actually messed up initially while rebasing. Hope it is fixed.

Shinyzenith commented 3 months ago

Thanks for the patch set!

InnocentZero commented 3 months ago

Thanks for the patch set!

My absolute pleasure :smile: Hope to contribute more significant patches in the future!