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 clap Derive API to accept command line arguments #247

Merged
merged 6 commits into from
Mar 27, 2024

Conversation

InnocentZero
Copy link
Contributor

@InnocentZero InnocentZero commented Feb 29, 2024

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 Shinyzenith requested a review from zubairmh February 29, 2024 19:39
swhkd/src/daemon.rs Outdated Show resolved Hide resolved
@Shinyzenith
Copy link
Member

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
Copy link
Contributor Author

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

@newtoallofthis123
Copy link
Contributor

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
Copy link
Contributor Author

InnocentZero commented Mar 19, 2024

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
Copy link
Contributor Author

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

@Shinyzenith
Copy link
Member

Devices

CC: @newtoallofthis123 How do you feel about this?

@newtoallofthis123
Copy link
Contributor

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
Copy link
Contributor Author

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

Glad to have it sorted, and sorry for the mixup 😅

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
Copy link
Contributor Author

@Shinyzenith can you go through this once? Thanks.

@Shinyzenith
Copy link
Member

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

@Shinyzenith
Copy link
Member

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

@InnocentZero
Copy link
Contributor Author

InnocentZero commented Mar 25, 2024

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
Copy link
Member

Shinyzenith commented Mar 25, 2024

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
Copy link
Contributor

newtoallofthis123 commented Mar 25, 2024

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

Shinyzenith
Shinyzenith previously approved these changes Mar 26, 2024
@Shinyzenith
Copy link
Member

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
Copy link
Member

Hi conflict still exists and now the commits are repeated.

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.

Signed-off-by: innocentzero <[email protected]>
This patch follows the previous one to update the method of taking
arguments for swhks. The derive API is the recommended method upstream.

Signed-off-by: innocentzero <[email protected]>
This patch fixes the conflicting short options in swhkd and implements
alternative short option forms.

Signed-off-by: innocentzero <[email protected]>
This change is meant to make the changes backwards compatible for the
user.

Signed-off-by: innocentzero <[email protected]>
@InnocentZero
Copy link
Contributor Author

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

swhkd/src/daemon.rs Outdated Show resolved Hide resolved
@Shinyzenith Shinyzenith merged commit b0fe88c into waycrate:main Mar 27, 2024
7 checks passed
@Shinyzenith
Copy link
Member

Thanks for the patch set!

@InnocentZero
Copy link
Contributor Author

Thanks for the patch set!

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants