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

config_to_str() ignores options with empty strings #537

Closed
andioz opened this issue Nov 18, 2020 · 11 comments
Closed

config_to_str() ignores options with empty strings #537

andioz opened this issue Nov 18, 2020 · 11 comments

Comments

@andioz
Copy link

andioz commented Nov 18, 2020

Hi there,

I'm experimenting with CLI11 version 1.9.1. I used boost::program_options for some years now and I'm looking for something smarter, with less cryptic syntax and less boilerplate code. Typically I offer a --print option in my programs, to print the current configuration to the console, and a --config option for reading such an config file in.

This works so far, but I have one problem, and cannot find a simple way to solve this. I want all options in the configuration file, including default values and empty strings. But the latter are not printed. Furthermore, there is a different format used for single character strings. Here is the simple example with my extensions:

// Copyright (c) 2017-2020, University of Cincinnati, developed by Henry Schreiner
// under NSF AWARD 1414736 and by the respective contributors.
// All rights reserved.
//
// SPDX-License-Identifier: BSD-3-Clause

#include <CLI/CLI.hpp>
#include <iostream>
#include <string>

int main(int argc, char **argv) {

    CLI::App app("K3Pi goofit fitter");

    app.add_flag("-p,--print", "Print configuration and exit")->configurable(false); // NEW: print flag

    std::string file;
    CLI::Option *opt = app.add_option("-f,--file,file", file, "File name")->capture_default_str(); // NEW: capture_default_str()

    int count{0};
    CLI::Option *copt = app.add_option("-c,--count", count, "Counter")->capture_default_str(); // NEW: capture_default_str()

    int v{0};
    CLI::Option *flag = app.add_flag("--flag", v, "Some flag that can be passed multiple times")->capture_default_str(); // NEW: capture_default_str()

    double value{0.0};  // = 3.14;
    app.add_option("-d,--double", value, "Some Value")->capture_default_str(); // NEW: capture_default_str()

    CLI11_PARSE(app, argc, argv);
    
    if (app.get_option("--print")->as<bool>()) { // NEW: print configuration and exit
        std::cout << app.config_to_str(true, false);
        return 0;
    }

    std::cout << "Working on file: " << file << ", direct count: " << app.count("--file")
              << ", opt count: " << opt->count() << std::endl;
    std::cout << "Working on count: " << count << ", direct count: " << app.count("--count")
              << ", opt count: " << copt->count() << std::endl;
    std::cout << "Received flag: " << v << " (" << flag->count() << ") times\n";
    std::cout << "Some value: " << value << std::endl;

    return 0;
}

Here is the output for different values:

$ examples/simple -p # 1
count=0
flag=false
double=0
$ examples/simple -f / -p # 2
file='/'
count=0
flag=false
double=0
$ examples/simple -f /tmp -p # 3
file="/tmp"
count=0
flag=false
double=0
$ examples/simple -f "" -p # 4
file=""
count=0
flag=false
double=0
$ examples/simple -f "/" -p # 5
file='/'
count=0
flag=false
double=0

I would like to have:

  1. file="" but it is missing completely
  2. file="/" but I get single quotes
  3. file="/tmp" this is fine
  4. file="" surprise! This looks like expected
  5. "/" another surprise, same as number 2

Is there a simple way to get the expexcted output? Or do I have to write a custom Config implementation for that?

@phlptp
Copy link
Collaborator

phlptp commented Nov 25, 2020

I will look into it a little more today

@andioz
Copy link
Author

andioz commented Nov 25, 2020

I did some investigations, using the JSON custom config implementation. This one behaves similar regarding empty strings. I got the impression the problem comes from using strings as internal representation for values, and string::empty() as indicator for non existing values. This takes away the chance to treat empty string values as normal values.

I think values should be stored as some "optional" type, not overriding the meaning of an empty string. But this would require a major redesign I guess. Or do you see a simple solution/workaround?

BTW; I didn't find any information about the native value type in an option instance. I think this is the reason why JSON config example generates only string output for config files, not native values like true/false for booleans, etc?

@andioz
Copy link
Author

andioz commented Dec 2, 2020

@phlptp, did you find some chance to look into it?

@phlptp
Copy link
Collaborator

phlptp commented Dec 4, 2020

I have not yet, but it is next on my list after finishing #545

@phlptp
Copy link
Collaborator

phlptp commented Dec 4, 2020

Well correction I did look into it earlier but haven't resolved anything regarding it yet. So I will take another crack at it soon

@phlptp
Copy link
Collaborator

phlptp commented Dec 6, 2020

Been messing around with this
https://github.com/phlptp/CLI11/blob/quoteCharacterSpecification/examples/config_app.cpp

This is where I am at currently in that I have a way in that branch to do everything specified in this Issue. I need to clean up a few things and add documentation before it is ready for a PR though, so probably a couple days before I get to polishing it up.

The main config follows toml specification and a distinction between strings and single characters which is where that default comes from, so I don't really want to change that. What I added was an ability to modify the config formatter to specify the exact quote characters to use in those contexts which means you can set them both to use single quote or double quote or some combination of them.

Regarding the default, if the default string given is empty that is the equivalent to clearing any default value, and there needs to be a way to do that so can't really be modified without adding a few functions and other structures. But the exception to that was if the option callback is forced, which would mean even if the argument wasn't given the callback for the option is used. In which case the default is used whether or not it was given. The modification was to extend this to the config output as well, so it will print regardless of what the default is.

I added the example and several tests about it to the cmake testing configuration.

@andioz
Copy link
Author

andioz commented Dec 6, 2020

Hey, many thanks for doing such serious and deep investigation. I just read your comment, will check details later, quite busy right now. I'm curious if/how I can solve this with your extensions!

@phlptp
Copy link
Collaborator

phlptp commented Dec 6, 2020

the config app example in that branch is basically the example you supplied with the minor tweaks to make the output the one you desired. Namely setting the config_formatter quote characters and adding the always run callback to the file option.

@andioz
Copy link
Author

andioz commented Dec 10, 2020

Hi again,sorry for the delay. I looked at your branch now and can confirm that all cases works now as expected. As you mentioned, there are some changes in the code base and in the example code required.

Some notes from a naive user: requiring run_callback_for_default() is not really intuitive, right? app.get_config_formatter_base()->quoteCharacter('"', '"') is new, assume this is required because single character strings are assumed to be characters. OK, it wwill work and it will solve my use case, hope you can push it into the next release.

Overall I understand that making bigger changes is not so easy! The concept using empty strings for absent values is not optimal, maybe, but anyway, it works. Thanks!

I let the issue open, you close it after merging I guess.

@andioz
Copy link
Author

andioz commented Dec 12, 2020

Hi, I found another edge case:

$ build/examples/config_app -p -f 1
file=1
count=0
flag=false
double=0
$ build/examples/config_app -p -f "1"
file=1
count=0
flag=false
double=0

In this case, the string is like a number, but should be a string. I guess this is more complicated...

@andioz
Copy link
Author

andioz commented Aug 25, 2021

I just tried version 2.0.0, the empty string problem is solved with it! Many thanks, great work!

@andioz andioz closed this as completed Aug 25, 2021
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

No branches or pull requests

2 participants