Should XFAIL_IF need parentheses?

[cbm755]

Regular if does not need parentheses (although I often put them anyway).

Which of these would you expect to work?

1. XFAIL_IF(foo()) (currently ok)
2. XFAIL_IF (foo()) (we should allow this, its possibly broken/untested currently, FIXME)
3. XFAIL_IF is_some_test(42)
4. XFAIL_IF DOCTEST_MATLAB

We could go with your idea of forbidding % and # from appearing in the directive conditional (and damn those who wish to printf in there). This would simply the regex a bit, perhaps have fewer corner cases to worry about (e.g., your various comment cases mentioned earlier).

[catch22]

Counterquestion: Our tests (as well as my code) seem to use the idiom
... % doctest: -ELLIPSIS % doctest: +XFAIL
rather than
... % doctest: -ELLIPSIS +XFAIL
Possibly this was because I felt that XFAIL/SKIP "semantically speaking" applies to the entire doctest (including the ELLIPSIS option), where as the latter seems to put the two directives on equal footing. Or perhaps it was just that the latter syntax did not work... 😄

In general I would only expect 1+2 to work. If we support the latter syntax then 3+4 seem problematic. Otherwise I have less strong of an opinion and would go for whatever is most robust to parse (but perhaps not document 3+4?).

I concur that breaking on % and # should be lead to a behavior that will lead to fewer corner cases.

[cbm755]

% doctest: -ELLIPSIS +XFAIL: we should do what Python does... (?) But I don't know what that is right now.

While we're listing stuff: its not obvious to me how where "and/or" goes in:

% doctest: +XFAIL_IF (foo)   % dcotest: +XFAIL_IF (bar)

(and similarly for *_IF and *_UNLESS). I much prefer:

% doctest: +XFAIL_IF (foo && bar)

[catch22]

I just checked and Python supports both of the following syntaxes:

>>> foo bar baz # doctest: +ELLIPSIS, +NORMALIZE_WHITESPACE

and

>>> print range(20) # doctest: +ELLIPSIS
...                 # doctest: +NORMALIZE_WHITESPACE

PS: I'd be fine with deviating from Python's path if there is a clearly more idiomatic Octave way of doing things.

[catch22]

Re your other question, I remember running into the same problem a while ago. Let me try to find a reference.

[catch22]

So our implementation calls DOCTEST__join_conditions, which combines the expressions by using ||. Thus a test will be skipped if and only if any of the +SKIP_IF conditions are true.

We briefly touched upon this subject in #112 but there was no conclusive reasoning. I'd say that it is perhaps the more intuitive behavior: +SKIP_IF(foo) always implies that the test is skipped if FOO is true - whatever the context.

So your changes will enable combining conditions via && without creating auxiliary helper functions/flags!

[cbm755]

% doctest: +SKIP_IF(foo) % doctest: +SKIP_UNLESS(foo)
I'm just trolling, I don't really think we need to resolve all these!

[catch22]

😄 Clearly we also need SKIP_ONLYIF!

[cbm755]

Seems the important decision is whether we support these:

% doctest: +ELLIPSIS, +NORMALIZE_WHITESPACE

% doctest: +ELLIPSIS, +NORMALIZE_WHITESPACE, +XFAIL_IF (foo)

% doctest: +ELLIPSIS +NORMALIZE_WHITESPACE

% doctest: +ELLIPSIS +NORMALIZE_WHITESPACE +SKIP_UNLESS (bar)

It does seems nice and clean to be able to combine them...

[cbm755]

breaking on % and # should be lead to a behavior that will lead to fewer corner cases.

I filed a separate bug #144.

[cbm755]

I'm unsure how to extend my regexp to support:

% doctest: +XFAIL_IF (foo())  +SKIP_IF (bar)

The regexp will glob foo()) +SKIP_IF (bar as the argument of the first directive. I suppose I could try to disallow things that look like non-nullary directives inside the argument:

[\+-]\w*((_IF)|(_UNLESS))

then need backtracking etc...

[cbm755]

Maybe a better approach would be to first break-up the string based on a regexp looking for DIRECTIVEs and comment chars. Then, in a second step, everything between those tokens must be either whitespace or arguments to non-nullary directives.

I think that might take me a while: I'm not going to try it now.

An much simpler approach would be to tell people they must not use whitespace inside the arguments:

% doctest: +XFAIL_IF(foo(42)&&bar||baz)   +SKIP_IF(true)  -ELLIPSES

(although doctests should be readable!)

[catch22]

I agree that this all sounds a bit involved. Python has it easy with its ast module!

I think the % doctest: ... % doctest: ... syntax is perhaps okay for now until someone feels sufficiently bothered by it to implement a patch! 😄

[cbm755]

Sounds good. And the answer to this particular issue is "Yes". Closing.