search

Azure / Microsoft365R

R SDK for interacting with Microsoft 365 APIs
Other
313 stars 44 forks source link

Can't call list_sharepoint_sites without attaching package first #72

Closed r-ash closed 3 years ago

r-ash commented 3 years ago

Hi, we'd like start using this package to replace one we wrote in house but we're running into an issue when using it without attaching it first. The use case here is I want to write a package which will import this one but I think that won't be possible currently as the package needs to be attached to work.

This is similar to #56

> site <- Microsoft365R::list_sharepoint_sites(site_url = "https://example.sharepoint.com/sites/mysite", tenant = "mytenant", app = "<app-id>")
Loading Microsoft Graph login for tenant 'example.onmicrosoft.com'
Error in make_basic_list(self, "followedSites", filter, n) :
could not find function "make_basic_list"
> traceback()
2: login$get_user()$list_sharepoint_sites()
1: Microsoft365R::list_sharepoint_sites(site_url = "https://example.sharepoint.com/sites/mysite",
tenant = "mytenant", app = "<site-id>")

This is due to how additional methods are added to az_user object https://github.com/Azure/Microsoft365R/blob/6fe14ea64e5ca78ccd313a05d332e88c799e13c4/R/add_methods.R#L165 . If Microsoft365R is not attached then make_basic_list does not exist in the environment so throws an error. This will be the same for other methods added to other objects from AzureGraph.

Is there a reason for the current design? Have you thought about adding additional methods to the AzureGraph objects using inheritance or dependency injection? I'd be happy to work on a PR if helpful.

hongooi73 commented 3 years ago

In general, you don't need to attach Microsoft365R to use it. The exception is make_basic_list, and the reason is because when you add a method to an AzureGraph class, it doesn't change the fact that the class still lives in the AzureGraph namespace. Hence when the Microsoft365R method list_sharepoint_sites calls make_basic_list, it will first look in the AzureGraph namespace for it. Since it doesn't exist there, it then looks in the global namespace. It never looks in the Microsoft365R namespace.

Basically the way that R6 objects interact with namespaces (which were really designed with S3 objects in mind and people attaching 1000 packages routinely) is wonky. Another example is how adding new methods via $set() has to happen at runtime in the onLoad function, rather than at build time.

make_basic_list was exported in version 2.2.1 as a quick fix for version 2.2.0, which was broken for this reason. There's probably better ways to do it; happy to take a PR.

Note though that the requirement to attach Microsoft365R shouldn't be a blocker for your package; you can simply put it in the Depends line in your DESCRIPTION file rather than Imports. If your package is for internal use only, noone is likely to complain. If you want to put it on CRAN, you might get some pushback, but there are still plenty of CRAN packages that have Depends dependencies.

hongooi73 commented 3 years ago

Also, if you want to make a PR and you use testthat: be aware that it won't pick up issues like this, since it makes everything in your package namespace globally visible. Relevant issue

hongooi73 commented 3 years ago

d4c6a1d5eb0f682d1393dc5c17b912447d4881ce

hongooi73 commented 3 years ago

Just realised I forgot to add you to the list of contributors in the DESCRIPTION file; I've fixed that. Thanks again for your PR!