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

[wip] autoload .oct file functions #180

Conversation

mtmiller
Copy link
Collaborator

@cbm755 something like this, works for me with all of my test cases.

Side effect being, if you doctest specific_function, and that function is also the name of a .oct file, it will also test all the other functions in the .oct file.

For all compiled function names, check Octave's autoload list for any
entries matching the .oct file name.  If any matches are found,
recursively test those function names.

Fixes gnu-octave#178.
@catch22
Copy link
Collaborator

catch22 commented Mar 11, 2018

Thanks Colin & Mike for working on this.

@cbm755
Copy link
Collaborator

cbm755 commented Mar 13, 2018

@mtmiller I'm sorry, I didn't see this before trying it myself!

if (strcmp(type, 'octfile'))
len = numel (what) + 4;
list = autoload ();
pmatch = @(e) (numel (e.file) > len) && strcmp (e.file(end-len+1:end), [what '.oct']);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this only matches the name of the oct file: what if an oct file has same name in a different path? In my patch, I used pwd/what but I'm not sure that's right either---should doctest meh.oct work if meh.oct is in the path but not in pwd? Maybe your way is less risky...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this only matches the basename of the .oct file.

I didn't want to use pwd because the user could be testing any .oct file in the load path, right?

It's true that this is a weaker check, a stronger check might be to get the full path to the .oct file with which or file_in_loadpath?

@@ -26,7 +26,7 @@
type = 'texinfo';
elseif (exist (what) == 3) % .oct/.mex
[~, what, ~] = fileparts (what); % strip extension if present
type = 'function'; % then access like any function
type = 'octfile'; % then access like any function
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in my patch, I made a distinction b/w doctest foo and doctest foo.oct: whereby the former tests only the function called foo and the latter tests all functions in foo.oct (including foo itself). Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I like your approach better.

len = numel (what) + 4;
list = autoload ();
pmatch = @(e) (numel (e.file) > len) && strcmp (e.file(end-len+1:end), [what '.oct']);
idx = find (arrayfun (pmatch, list));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this arrayfun approach probably nicer than my approach! Nice shortcut with numel.

@cbm755
Copy link
Collaborator

cbm755 commented Mar 13, 2018

Note for the record: there is no return after the recursion, this is because doctest foo.oct needs to test all the things from the autoload map as well as "help foo" itself (which AFAICT is not listed in the autoload map---see e.g., __py_struct_from_dict__.oct from Pytave).

@cbm755
Copy link
Collaborator

cbm755 commented Mar 13, 2018

@mtmiller I'm indecisive about using recursion here (like you've done here) or a loop to match how classes behave.

@mtmiller, maybe you can review my patch in #179 too and we can decide what to merge after that?

@mtmiller
Copy link
Collaborator Author

Seems like we converged on almost the same approach, I'll review your branch and maybe you can incorporate some of my differences into yours.

@cbm755
Copy link
Collaborator

cbm755 commented Mar 19, 2018

I'll close this as we're converging our commits at #179.

@cbm755 cbm755 closed this Mar 19, 2018
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

Successfully merging this pull request may close these issues.

3 participants