brndnmtthws / conky

Light-weight system monitor for X, Wayland (sort of), and other things, too
https://conky.cc
GNU General Public License v3.0
7.27k stars 620 forks source link

Named arguments #1153

Closed missing-semi-colon closed 11 months ago

missing-semi-colon commented 2 years ago

I was trying to add a new argument to the graph but realised it would be quite complicated with the current way arguments are parsed, so I decided to implement named arguments to make this simpler. I am wondering if this is something that others want added to conky, in which case I may spend more time on it to expand it to work with widgets other than the graph and add documentation.

This allows things like: cpugraph cpu1 50,50 --last_colour 00FF00 --first_colour FF0000. Argument names and values can be separated by either spaces (--key val) or an "=" (--key=val). Note that named arguments override positional arguments, so cpugraph cpu1 50,50 --height 100 would have a height of 100.

It is backwards compatible, so should not break current configs.

See https://github.com/missing-semi-colon/conkyer/commit/4d6b863816ba04a2432cb9da7e5d81e4ecffeb7b for the implementation. See https://github.com/missing-semi-colon/conkyer/commit/56f1fd65f7f905195839d347eded321f809fa8ab for an example of adding a new named parameter.

Sorry for any obvious issues with my code, I am new to C++.

Update: named arguments available for bars, gauges and graphs.

humanoid2050 commented 2 years ago

Interesting idea. Is there any requirement that named arguments always come after unnamed arguments (a la python)? Do named arguments only apply to optional args, or all args?

missing-semi-colon commented 2 years ago

Yes, named arguments must come after unnamed arguments

I am not sure what you mean by optional args because as far as I'm aware (at least with the different graphs) all args are optional and default values are used where no value is given. However named arguments do not apply to all args, only the ones that are general for all graph types. So as an example, for cpugraph it is not possible to specify a value for cpuN with a named argument because it is specific to that graph type. This can probably be changed, but I'm not sure how difficult it would be.

humanoid2050 commented 2 years ago

I think I misunderstood. For some reason I thought you were proposing the ability to name all args for all conky variables, not just graph-related args.

I agree the sscanf approach to arg parsing is a bit fragile, especially if you want to add new parameters. Is it possible to extend the optional named args pattern to all variables?

missing-semi-colon commented 2 years ago

Extending it to support bars and guages would be straight forward.

Extending it to other variables that have multiple parameters I think is possible but a bit more difficult. I don't think it would be useful to extend it to variables that only have 1 parameter though.

humanoid2050 commented 2 years ago

It does seem like a python-style approach might work well. Functions would have a specific expected ordering to their arguments. If that ordering works for the caller, they can simply supply values and they will be interpreted sequentially. If the caller wants to use a different order when calling the function or skip arguments, they must indicate the deviation from the sequence by using named arguments. Once a named argument is used, all the remaining arguments provided must be named as well.

Of course, the would need to be balanced with the sheer amount of code and documentation that would need to be updated to fully implement this pattern.

For my own part, I am interested in where this goes because I had to make an alteration to the scan_bar method to support supplying a scale value via the arg string. You can see that here here.

missing-semi-colon commented 2 years ago

Going completely Python, in that positional arguments must be given in a single specific order, would require breaking compatibility with older config files because currently positional arguments can be given in multiple permutations. Maybe the Python way would be better but I didn't want to break backwards compatibility.

I see this change as being most useful when a command has a large number of parameters. Currently positional arguments are fine when there is only a few available, and named arguments can be included just to give some choice to the user. However the biggest reason I implemented named arguments was due to the complexity and amount of repetitive code when there is a large amount of positional arguments. In this case, when wanting to add more arguments I see it better to not add any more positional arguments and only add named arguments as these are simpler to add.

I have also made an update so that bars and gauges can use named arguments as well as graphs, https://github.com/missing-semi-colon/conkyer/commit/4d6b863816ba04a2432cb9da7e5d81e4ecffeb7b.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 365 days with no activity. Remove stale label or comment, or this issue will be closed in 30 days.

github-actions[bot] commented 11 months ago

This issue was closed because it has been stalled for 30 days with no activity.