doctest @logical versus doctest logical

[cbm755]

The symbolic package which defines @logical/isAlways.
There is some distinction between doctest logical and doctest @logical

>> pkg load symbolic
>> pkg load doctest
>> doctest logical
Doctest v0.6.1: this is Free Software without warranty, see source.

logical ................................................ NO TESTS

Summary:

   PASS    0/0   

1/1 targets passed, 1 without tests.

>> doctest @logical
Doctest v0.6.1: this is Free Software without warranty, see source.

logical ................................................ NO TESTS
@logical/isAlways ...................................... PASS    2/2   

Summary:

   PASS    2/2   

2/2 targets passed, 1 without tests.

So far so good.

But if you're a developer, this distinction doesn't work if your pwd is octsympy.git/inst/: in that case both will do the same as doctest @logical...

Not sure how important this is, but developers are exactly the people using doctest...

[catch22]

Hmm. We would expect that doctest logical always picks out the built-in and doctest @logical always the class, right? It seems that one part of the issue is the following:

https://github.com/catch22/octave-doctest/blob/9320d2454ee1447f0373537a83748f764fe08e83/inst/private/doctest_collect.m#L33-L42

I think this should possibly be changed to:

elseif (exist(what, 'file') && ~exist(what, 'dir')) || exist(what, 'builtin')
    type = 'function';
elseif (strcmp(what(1), '@')) || (exist(['@' what], 'dir'))
    % comes after 'file' above for "doctest @class/method"
    type = 'class';

In fact, why don't we remove the exist(['@' what], 'dir' test so that the user has to add the @ prefix if they want to test a class?

[catch22]

A second, unrelated issue is that it seems confusing to me that we are testing both the class and the builtin when the user invokes doctest @logical. I think this is because in collect_targets_class we are trimming off the initial @ (https://github.com/catch22/octave-doctest/blob/master/inst/private/doctest_collect.m#L249), so we end up calling get_help_text('logical') (https://github.com/catch22/octave-doctest/blob/master/inst/private/doctest_collect.m#L274). I think we should only trim off the @ for the methods calls.

[cbm755]

I will try you suggestion...

In fact, why don't we remove the ... so that the user has to add the @ prefix if they want to test a class?

That is do-able. help sym only shows the constructor after all...

Cross referencing this with a discussion at apjanke/octave-testify#12 where @mtmiller asked about runtests myclass. It would be good if test and doctest someday eventually behaved similarly.

[mtmiller]

Yeah to me the @myclass looks like an implementation detail of the filesystem. It's not like classes are instantiated with x = @myclass. If the class is defined in a single myclass.m file with no @myclass directory, would you test it with doctest @myclass or with doctest myclass? What would the @ refer to in this case? What about a class in a package directory?

[catch22]

In Colin's case there is both a builtin and a class with the same name. Which of the two do you feel should take precedence when the users types doctest logical? The builtin, the class, or the one that appears first in the path?

I feel the current behavior (testing both the builtin and the class) is a bug in collect_targets_class and should be changed.

[mtmiller]

I think I really don't have enough experience and data with Matlab's naming conventions. I just played with the help system a little on Malab's web site. Some observations:

• Both help sym and help @sym return help on the constructor only
• help double returns the obvious, help @double returns a list of methods! How would I get that for sym?
• help logical returns the obvious, help @logical says nothing found
• help datetime and help @datetime behave the same as sym
• help containers.Map returns help for the constructor, help containers/@Map returns a list of methods

So far I don't see a lot of consistency, but it seems that the @ symbol does tend to refer to getting all methods of a class as opposed to just the constructor. Why doesn't that work for some classes? Does that also work with a user class defined in a single .m file not in a @-directory? I can't test that.

I think it does make sense to have some way to distinguish a class from a constructor, so this seems to be the way.

[cbm755]

both a builtin and a class with the same name.

Well, they are not really distinct. By implementing @logical/isAlways, Symbolic is adding methods to the existing (builtin) logical objects. There isn't a new class called logical (at least from the users point of view).

[cbm755]

How would I get that for sym?

methods double, that's what doctest is doing for doctest @double.

See also the older discussion #87

[cbm755]

Does that also work with a user class defined in a single .m file not in a @-directory? I can't test that.

I think common practice (e.g., Chebfun, biggest user of Matlab OO that I know of) is to still use @klass dir for a classdef class named klass.

[mtmiller]

methods double, that's what doctest is doing for doctest @double.

Sure, but is there a reason for help @myclass returning either a constructor or a list of methods in Matlab, depending on which class it is? This is getting off topic, but I was playing around to see if the @ prefix is a consistent notation for "the full contents of a class" vs the just constructor.

[mtmiller]

TL;DR I agree with the original issue here, doctest @logical is supposed to refer to all methods of the class, and doctest logical only to the built-in or function or constructor. Seems to me that is the intention that Matlab is going for, whether it's fully consistent or not is a separate issue.

[catch22]

Well, they are not really distinct. By implementing @logical/isAlways, Symbolic is adding methods to the existing (builtin) logical objects. There isn't a new class called logical (at least from the users point of view).

Oh, I see, so logical is also a "built-in class", not just a conversion function. (Or is the conversion function really a constructor and the documentation is imprecise?) And while built-in classes can have user-defined methods (like you @logical/isAlways) it seems one can only call `methods' when the user has actually defined methods for it:

> class(true)
logical
> methods("logical")
error ...

(When I am in octsympy's inst folder, methods returns isAlways.)

How very confusing 😄

[catch22]

To summarize, is this what we want to go for?

1. doctest @foo - all methods defined in the class
2. doctest @foo/bar - method bar of class foo
3. doctest @foo/foo - constructor of foo (special case of sorts of the preceding)
4. doctest foo - constructor of foo (shortcut of the preceding)

2-4 seems to agree with how help works. 1 is analogous to how our doctest dir tests all files in that directory.

[cbm755]

4. doctest foo test general class docs (often same as constructor but not always: help foo and help @foo/foo are not necessarily the same for classdef).

[mtmiller]

doctest foo test general class docs (often same as constructor but not always: help foo and help @foo/foo are not necessarily the same for classdef).

I don't understand this exception. Are they different because of a bug in Octave? Are they different because of some weird Matlab logic I don't understand yet?

[mtmiller]

1. doctest @foo - all methods defined in the class

I would hope that this can also be made to work with a classdef class defined in a single foo.m file, and whether or not it is in a directory named @foo.

[cbm755]

I don't understand the exception:

Classdef can have a block of docs at the beginning. In Matlab, this is returned by help myclassdef, distinct from `help myclassdef.myclassdef``

classdef myclassdef
%MYCLASSDEF  A test for classdef classes
%
%   Some tests:
%   >> 6 + 7
%   ans = 13
%
%   >> a = myclassdef()
%   a =
%   class name = "default", age = 42
%
%
%   This general help text should be shown for "help myclassdef".
%
%   There are also tests in the methods below.

  properties
    name
    age
  end

  methods

    function obj = myclassdef(n, a)
      % constructor
      % >> a = 13 + 1
      % a = 14
      if (nargin ~= 2)
        obj.name = 'default';
        obj.age = 42;
      else
        obj.name = n;
        obj.age = a;
      end
    end
  end
  methods
    function disp(obj)
      % >> a = 30 + 2
      % a = 32
      fprintf('class name = "%s", age = %d\n', obj.name, obj.age)
    end
  end
end

[cbm755]

I don't think the Matlab logic is weird in this case, although its true we don't support it yet.

[cbm755]

Back to @catch22's proposal: I'm not sure I agree yet. Maybe I prefer doctest foo to doctest everything about object foo.

Rational:

• User wants to know which methods apply to foo, she types methods foo (not methods @foo).
• User wants to know about a (modern classdef) class: her starting point is help foo: that documentation has appropriate links to other methods.
• Matlab's docs use "see also sym/assume".
• Matlab's help works like help sym/assume and help sym.assume. It is true that help @foo/bar sometimes works but not e.g., for help @sym/assume... and I've not seen this @ version documented.

Downside: doctest klassdef would test more than just what help klassdef shows.

This also reserves the idea that doctest @foo might explicitly test only methods defined in local directory (see #87), not superclass methods, not ones defined elsewhere in the path, etc. This is the current doctest behaviour but I think its sort of by accident...

[catch22]

I think I agree.

[cbm755]

Current summary of my thinking based on the above:

0. doctest klass will run everything it can find about class klass.

1. To test only the constructor do doctest @foo/foo (eventually doctest foo.foo).

2. doctest @klass might eventually do something more "local", like only those methods defined in the local subdir pwd/@klass (and not methods defined in superclasses or defined in other areas of the path).

3. As noted above help foo and help foo.foo are not the same thing for classdef.

4. Thus a remaining ambiguous case is if one wants to test the "top level" class docs of a classdef (help foo) but not the entire class. Perhaps doctest foo -nonrecursive will do this.

All of this should be tested with BIST to ensure we grab the correct number of tests in each case: I've started doing this in #198.

[apjanke]

I mostly agree with your thinking here as to how the test specifiers should work. Especially WRT item 0 and how the main usage should mean "everything it can find about class klass". I think the complication is that class or (or "type") definitions can be spread across multiple locations:

1. a classdef klass.m file
2. on or more @klass directories. Because M-code allows duck-punchng/monkeypatching, you need to consider all the @klass dirs on the path, not just the one that contains the constructor. They all contribute to the effective definition of the class
3. libinterp/liboctave core .cc or .tst files for types which have a built-in component to their definition

I think I prefer doctest @klass to doctest klass for this usage, because it's unambiguous that it's talking about the whole class. In other M-code contexts like which or function calls or references, an unqualified klass can refer to the class's constructor, not the entire class itself. But I don't think this will matter in practice; klass.klass could always disambiguate it by referring to the constructor.

We might want a doctest -file <whatever> switch to force it to interpret the input as a path to a file so you can unambiguously point at single directories that might otherwise be interpreted as class or function names.

like only those methods defined in the local subdir pwd/@klass (and not methods defined in superclasses

Is it normal for doctest to pull in all the superclass's tests, too? That seems a little unusual. If you run a test that tests a bunch of classes in a class hierarchy, you might end up redundantly calling the tests on superclasses. Especially if you have some real basic abstract base classes or mixins that get used in a lot of places.

[cbm755]

I think I prefer doctest @klass to doctest klass for this usage

Do you mean "in theory you disagree with Item 0, but in practice you agree"? I think that summarizes how I feel :)

Is it normal for doctest to pull in all the superclass's tests

It does whatever methods(foo) does. On Matlab, that pulls in superclasses. On Octave is does not (but of course that might change for compatibility). This is issue #87. I agree pulling superclasses is not ideal, so its possible methods(foo) may not be the right tool for the job, long term. Feel free to discuss further at #87.

[apjanke]

Do you mean "in theory you disagree with Item 0, but in practice you agree"? I think that summarizes how I feel :)

I agree with the semantics; I just have reservations about the syntax. ;)

It does whatever methods(foo) does.

Could we change it to do meta.class.fromName(foo) and probe its .MethodsList for the non-inherited methods instead?

[catch22]

1. doctest klass will run everything it can find about class klass.

👍 With regards to testing the docstrings of inherited methods that are not explicitly overridden: What do you feel is the most intuitive default? We can also do what's most easiest to implement for now and specify that the behavior in this situation is undefined (for future fixing).

2. To test only the constructor do doctest @foo/foo (eventually doctest foo.foo).

👍 Should we already today use the syntax foo.bar for both old and new-style classes (or whatever is most idiomatic)?

5. Thus a remaining ambiguous case is if one wants to test the "top level" class docs of a classdef (help foo) but not the entire class. Perhaps doctest foo -nonrecursive will do this.

I think I like nonrecursive (even though in the present context it could also be interpreted as "do not collect methods from parent classes"...).

(Another option would be to invent a special target corresponding to the top level class docs, such as foo.DOCTEST).

[apjanke]

With regards to testing the docstrings of inherited methods that are not explicitly overridden: What do you feel is the most intuitive default?

I think ignoring inherited stuff is the most intuitive. But I've been working with OOP testing frameworks for a long time and that may color my view.

Should we already today use the syntax foo.bar for both old and new-style classes (or whatever is most idiomatic)?

I would say yes. The difference between old and new style classes is an implementation detail of how the source files are laid out on disk; I don't think the user cares about that. So I think the same syntax should be used at the doctest interface level for both old and new classes, and it should smooth over the differences behind the scenes. And the foo.bar syntax seems to be what meta.class.fromName and which are using in Matlab, so I'd say that's the most idiomatic way to reference a class or class method.

[cbm755]

use the syntax foo.bar for both old and new-style

Filed #203.

With regards to testing the docstrings of inherited methods that are not explicitly overridden: What do you feel is the most intuitive default? We can also do what's most easiest to implement for now and specify that the behavior in this situation is undefined (for future fixing).

Discussion for #87? I think the rough consensus is we'd like to test overridden methods only (but not only methods defined in pwd).

[catch22]

I think the rough consensus is we'd like to test overridden methods only (but not only methods defined in pwd).

I agree that this sounds most intuitive.

[cbm755]

I read through this thread, lots here. I think we do most of this now.

I think the rough consensus is we'd like to test overridden methods only (but not only methods defined in pwd).

I agree that this sounds most intuitive.

This is what we do now (at least on Octave). I think I'll close the thread: its all still here if we need to reference this in the future!