isLong() does not work as it is expected

[EVGVir]

Although asLong() returns a valid value for an integral value passed as an argument isLong() at the same time returns false.

It looks like such behaviour is connected with the correction #28: meaning of asLong() was changed but isLong() left unchanged.

In my particular case, there was a check if the passed value is a number (isLong()?): to have a guarantee that there will be no exception from asLong(). However, the check returned false on every occasion.

[jaredgrubb]

Note that you'll get long values for repeated options, not for options that take numbers. It's not really possible to specify that an option takes a number, just that it takes a string that you can try to convert to a number.

[EVGVir]

The statement 'you'll get long values for repeated options, not for options that take numbers' was true prio to #28. However, now asLong() is able to extract values for options that take integral values as well.

'It's not really possible to specify that an option takes a number' Is it needed at all? After all, the original library is written in Python. So, if something can be represented as a number then it could be used as a number (and checked, I presume). For this reason, isLong() that checks if a value is a number looks more consistent with the Python version.

There are just three cases for an option that takes an integer argument:

• nothing is passed to this option then isLong() can return false.
• a literal representation of an integral number is passed then isLong() returns true.
• a string that can't be converted into an integral value is passed then isLong() return false.

I doubt only about the first item. Is such behaviour okay in light of repeated options? If yes then what can be the reason for isLong() and asLong() desynchronization?

[jaredgrubb]

Yes, I can see the inconsistency. To me, it feels a little odd that "isLong" could have to do string parsing to give an answer, but I'm not sure I can think of a way around it. We could create a different set of functions for this purpose, but I think that's even less desirable.

So I think the only solution is to do as you say and make sure that "isLong" returns true if-and-only-if "asLong" is valid.

[Timmmm]

I too assumed that isLong() would just tell me whether asLong() was going to throw an exception. I feel like the entire value API is a bit weird actually.

Since the docopt description string is untyped there's no way for docopt to know whether the arguments should be strings or not until the user (of docopt) tries to use the values. So it's weird that the docopt::values have a preconceived idea of whether they are a bool, long, string or whatever. They can't possibly know that until they are queried. A more sane API would be something like this:

class value : public std::string
{
public:
    bool isInt() const; // Returns true if it can be converted to an integer successfully.
    int toInt(int def = -1) const; // Convert to int, or return `def` on failure.
    bool isBool() const; // Return true if it is "true" or "false"
    bool toBool(bool def = false) const; // etc.

    // And if you *insist* on using exceptions, remove the `= -1` and `= false` above and add:
    int toInt() const; // Convert to int, throw exception on failure.
    bool toBool() const; // Convert to bool, throw exception on failure.
};

That is way simpler and more obvious I think. However you may wish to wait for C++17 to be common and then it can be done properly: std::optional<int> toInt();

[isometriq]

indeed, had to do the conversion from the string value

int val = std::stoi(args["--param"].asString());
float val = std::stof(args["--param"].asString());

[isometriq]

Just to add, shouldn't there be a isEmpty()

I have this situation where an option is included by [options] then using count() i know the option is declared, but its kind is 0 (Empty). The API doesn't allow me to know, unless i call every is*** in a try/catch... did i miss something?

[isometriq]

It happens when option is not used and there is no default value defined.

[jaredgrubb]

Yeh, I'm not 100% happy with the 'value' class.

I like your idea about the forms with default values. I think that makes a lot of sense. And "isEmpty" is a good idea as well.

[indianakernick]

You could move conversion from string to long into a separate function.

bool isLong() const {
  return kind == Kind::Long;
}

long asLong() const {
  throwIfNotKind(Kind::Long);
  return variant.longValue;
}

// Returns true if asLong won't throw
bool convertToLong() {
  if (kind == Kind::Long) return true;
  if (kind != Kind::String) return false;
  
  const char *begin = variant.strValue.data();
  const char *end = begin + variant.strValue.size();
  
  long longValue;
  const auto [pos, err] = std::from_chars(begin, end, longValue);
  if (err != std::errc{}) return false;
  if (pos != end) return false;
  
  std::destroy_at(&variant.strValue);
  kind = Kind::Long;
  variant.longValue = longValue;
  return true;
}

I really don't like having asLong throw all kinds of different exceptions. None of them have the name of the option in them (because value doesn't have that information). It means that I have to do this if I want nice error messages:

long longValue;
try {
  longValue = value.asLong();
} catch (std::exception &) {
  std::cout << "--count must be an integer\n";
  return 1;
}

With convertToLong, I can do this:

if (!value.convertToLong()) {
  std::cout << "--count must be an integer\n";
  return 1;
}
long longValue = value.asLong();

Would something like this be accepted in a PR? I'm asking because std::from_chars is C++17 (so is std::destroy_at for that matter but that can easily be replaced with variant.strValue.~basic_string()). While I'm at it, I could address #115 and do a general refactoring of the code. BTW, why is it value and not Value?

---

Another approach that uses exceptions with better error messages might be possible by putting the option name in value. Something like this:

struct Option {
  std::string name;
  Variant variant;
  Kind kind;
};

// We provide less-than comparisons for option and strings
// to make options.at("--option") work
struct OptionLess {
  struct is_transparent {};

  bool operator()(const Option &a, const Option &b) const {
    return a.name < b.name;
  }
  bool operator()(const Option &a, const std::string &b) const {
    return a.name < b;
  }
  bool operator()(const std::string &a, const Option &b) const {
    return a < b.name;
  }
};

using Options = std::set<Option, OptionLess>;

Then we get better error messages:

value.convertToLong(); // could throw "--count must be an integer"
long longValue = value.asLong();

Though, I'm not really sold on putting the name and value into one class. I should also mention that the is_transparent thing is C++14. Is this a good idea?

[indianakernick]

@jaredgrubb Unrelated, but what's the use-case for value::hash()?

[jaredgrubb]

hash

This is useful so the value can be put inside of hash-map. I remember wanting that for some reason, but don't know now :)

I like the idea of convertAsLong but I'm nervous about the side-effect (eg, a=b; a.convertAsLong(); bool equal = (a==b); /* is true? */).

What about a value.asLongOr(defaultValue) function?

I appreciate your ideas and where you're trying to go! I think we'll find something to help.

[indianakernick]

@jaredgrubb I see your point about the side-effects but I don't think that would be a problem given the usage of the function. I don't really do a lot of "processing" with docopt::value. I just extract the raw value out as soon as possible and do all the work with that.

asLongOr seems OK. Here's a comparison:

if (!value.convertToLong()) {
  // handle error
}
long longValue = value.asLong();


long longValue = value.asLongOr(-1);
if (longValue == -1) {
  // handle error
}

I think convertToLong is a bit cleaner though.

[jaredgrubb]

Well, that's not the only one (eg, the hash value will change).

Maybe we should fix isLong to handle both cases?

[indianakernick]

Maybe we should fix isLong to handle both cases?

I'm not sure what you mean. Could you explain?

[indianakernick]

I've been thinking about this for a while and the side effect of convertToLong really shouldn't be a problem as long as you check the return value. The return should probably be [[nodiscard]] to enforce this.

[[nodiscard]] bool convertToLong() {
  // ...
}

The side effect should make perfect sense if you check the return value.

if (value.convertToLong()) {
  // value.isLong() is true
  // value.asLong() won't throw
} else {
  // value.isLong() is false
  // value.asLong() will throw
}

Ignoring the return value is a really bad idea.

value.convertToLong();
long longValue = value.asLong(); // this may or may not throw. Who knows?

[jaredgrubb]

isLong could return true if and only if the call to asLong will succeed.