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

Improve usage message printing #877

Merged
merged 2 commits into from
Oct 29, 2022
Merged

Improve usage message printing #877

merged 2 commits into from
Oct 29, 2022

Conversation

yshui
Copy link
Owner

@yshui yshui commented Aug 21, 2022

No description provided.

@codecov
Copy link

codecov bot commented Aug 21, 2022

Codecov Report

Merging #877 (59de702) into next (2dae094) will decrease coverage by 0.32%.
The diff coverage is 12.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next     #877      +/-   ##
==========================================
- Coverage   37.97%   37.64%   -0.33%     
==========================================
  Files          48       48              
  Lines       10567    10641      +74     
==========================================
- Hits         4013     4006       -7     
- Misses       6554     6635      +81     
Impacted Files Coverage Δ
src/dbus.c 21.71% <0.00%> (ø)
src/picom.c 66.95% <ø> (-0.62%) ⬇️
src/options.c 19.71% <11.84%> (-1.64%) ⬇️
src/diagnostic.c 75.00% <100.00%> (ø)
src/win.c 67.47% <0.00%> (-0.82%) ⬇️

@tryone144
Copy link
Collaborator

Thanks for simplifying the options interface! This has been bugging me for a while. 😅


I have some experimental code locally that uses an enum for the option id's / shortcodes instead of relying on the magic numbers. Having that would be a nice addition when extending/restructuring the options.

However, I am still struggling to find a nice way of merging the general handling in options.c and config_libconfig.c. Having the same logic in two places is kind of not nice. The first attempt using macros has turned out to be even messier...

@yshui yshui requested a review from tryone144 August 21, 2022 22:39
@yshui
Copy link
Owner Author

yshui commented Aug 22, 2022

@tryone144 can i see your first attempt?

@tryone144
Copy link
Collaborator

@tryone144 can i see your first attempt?

I am currently away from my main machine on a spotty (at best) internet connection. I will push the partially working WIP code as soon as I get back.
Code-reviews might be delayed as well. Unless you are in a hurry to merge the latest PRs, I recon I'd get these done around the next weekend.

@tryone144
Copy link
Collaborator

@tryone144 can i see your first attempt?

See this branch. This is out-of-date with next and might not compile.
Notable changes are:

  • enum over all configuration options in options_defs.h to get rid of manually assigned ids.
  • do all range-checking and clamping in options.c and only there.
  • preprocessor macros for handling int, long, double, bool, and string arguments in options_macros.h. The idea was to define appropriate macros for getopt and libconfig and then include this header as the single source of truth (I think the approach is called X Macros). Ended up being quite messy in the current state and not really a step forward; particularly due to the edge cases and special handling some options require.

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
Laying the usage message out by hand is tedious, also error prone
because the option names are duplicated at 2 places and have to be
consistent.

Create a struct to hold the option names and help messages, and
do layout programmatically.

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
@yshui
Copy link
Owner Author

yshui commented Oct 29, 2022

@tryone144 I've looked at your attempt. Nice idea but it did end up being quite messy. I think we can merge this PR as an incremental improvement. Eventually I think it might be a good idea to generate the config code from a python script or something.

@yshui yshui merged commit 5ec79a0 into next Oct 29, 2022
@yshui yshui deleted the help-message branch October 29, 2022 18:52
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