Added rudimentary S3 support

[J-Moravec]

Added rudimentary S3 support for import::from using the .S3 argument, resolving the issue https://github.com/rticulate/import/issues/58.

The code doesn't do a lot of checking so I am not sure if something might not break. But tests are passing.

There is also an issue that what is generic and what is a class name might be ambiguous. For example:

t is a generic with t.default and t.data.frame methods.
t.test is also a generic, with t.test.default and t.test.formula methods.

A hypothetical t.test.data.frame might then be a t generic for the test.data.frame class, t.test generic for the data.frame class and t.test.data generic for the frame class and there is no easy way to resolve this ambiguity.

So this code is working for simple problems, when bugs are found, more complex tests can be designed and the code can be refined.

[torfason]

That's great! The functionality, based on the tests, looks nice.

Checks do not pass on master because of an issue with the check action itself (since fixed on dev). I rebased your branch onto dev, fixing one conflict, and pushed to prx/J-Moravec/s3_support (prx indicating temporary branch).

Checks still fail on that branch, though, but now look to be related to the new S3 feature, which is always good :-).

If you would like to take a look at that and see if it is clear to you what is going wrong, one approach would be to fork/clone that branch to your own repo, which should trigger the checks on push. Alternatively, if you rebase your original branch onto dev (fixing the conflict, which has to to with .all_names=TRUE (see 00f8c9d0ed6c5ce2e971c0badabdd7f6239caa92)), you could update the PR which should also trigger a check.

I'm traveling so it will be a bit until I have a chance to look closer at it myself.

[J-Moravec]

Thanks. I rebased this to the dev branch, fixed merge issues, changed get into base::get and fixed the issue that caused the strange error deep in isS3method by first checking if the object is a function.

For some reason, testthat is broken with import and produces an unreadable mess instead of a trace.

[torfason]

Close and reopen to trigger checks.

[torfason]

Nice, checks seem to be passing apart from some hang issue that is probably not important, and the diffs with dev look nice and clean. I'll check out the functionality locally later this week.

The testthat output is a known issue. Standard testthat does not work with import (some complicated interaction between the way the two packages deal with environments). So the tests run in a very rudimentary custom harness, that has bad output on failure. Sorry about that. Works well if run locally inside the interactive environment, but yes, issues that come up only on the GitHub actions can be a pain to debug. But enhancing the harness is too much trouble to bother with it.

[J-Moravec]

Well, the issue is that it produce unevaluated decoration for testthat reports even locally, which took an extra time to find what is going on. This was also compounded by the error thrown out when my terminal version was not detected properly thanks to testthat migrating from crayon to cli. Maybe time for me to finally migrate to tinytest instead. Eh. But its fine, no worries.

I added .S3 par to here and into with respective tests. Happy to look into other issues.

[torfason]

Yes, tinytest looks very interesting. If that allows eliminating the custom harness that seems totally worth migrating (although probably post-1.3.0).

[torfason]

OK, I had a better look at this and I think we should be able to include it. There are some things that are a bit different from the way the rest of import works – most noticable is the way that it vacuums up any .S3 functions found in the source file, rather than look at the contents of the ... to determine what to import.

But, all of this is very nicely insulated behind .S3=TRUE and if this is not specified it looks like no functionality should be affected. So rather than overdesign this, let's see how the usage of this evolves this and what functionality people would like.

Therefore, what I propose is to mark this parameter as experimental, stating that people should use it and report on how it goes, but that syntax and semantics for S3 functionality may change in the functionality. I've actually done this already, in prx/J-Moravec/s3_methods

Beyond that, what needs to be done is:

• [ ] Document the internal functions as well, with a @noRd (see for example make_import_call.R)
• [ ] Reformat the code in line with the import package style (two space indents, curlies on own line, etc., see from.R for reference)
• [ ] Possibly write a section in the vignette - optional, given that this is a first version, but could increase adoption.
• [ ] Rebase into three commits (Test, Implement, Document) – I'd be happy to do this if you are not into rewriting git history.

[J-Moravec]

@torfason

[x] Documented internal functions [x] cleaned style [x] wrote a small section in vignette [x] rebased into three commits: tests, implementations and documentation

Could you please check again that everything is in order? Thank you very much.

[torfason]

Looks great, this is now on dev for inclusion in the upcoming 1.3.0 release :-)