wahani / modules

Modules in R
https://cran.r-project.org/package=modules
Other
81 stars 3 forks source link

Error when exporting generic overload of operator (e.g. "==") #43

Closed NilsRodrigues closed 1 year ago

NilsRodrigues commented 1 year ago

As of version v0.10.1 there is code to handle exports with special characters #37 and spaces #39. The codes adds quotes `` to escape these names of exports.

However, when trying to export overloads of generic functions (e.g. ==.foo, !=.foo) for S3 classes, there are no quotes and the export fails. Is it possible to fix the export in order to handle such overloads for S3 classes?

Is there a reason why not all names are quoted to allow any arbitrary characters? There is a comment in the corresponding line of source code in the modules package that says "Exclude Linting"...

Code to reproduce error: modules::module({"==.foo" <- function(left, right) {return(left, right)}})

Thank you for your time and effort.

wahani commented 1 year ago

Hi. Thanks for reaching out.

The reason for this is the export mechanism of modules. At some point I decided that it would be a good idea to be able to rename objects while defining exports. The motivation was that we may want to export only one element of a sub-module under a different name. So these days we can do something along the lines of

modules::module({
  export(myNewExport = base::mean)
})

So exports in this way are just R expressions. Essentially you may just define a function inside a call to export. Now this requires us to distinguish between R expressions, which we do not want to wrap in `, and symbols that we do want to wrap in `. In the place in the code you reference we would need to find a regular expression that can reliably distinguish between these cases. So we may add something like 'strings that begin with punctuation' (^[[:punct:]].*).

But since we have already a powerful (and slightly complex) export mechanism, you may also try

m <- modules::module({
  export("==.foo" = equals)
  equals <- function(left, right) {return(left == right)}
})

m$"==.foo"(1, 2)

Would this already be of help?

wahani commented 1 year ago

In the place in the code you reference we would need to find a regular expression that can reliably distinguish between these cases. So we may add something like 'strings that begin with punctuation' (^[[:punct:]].*).

So this is never that easy without breaking current behavior. E.g.

m <- modules::module({
  export(true = !FALSE)
})

would not survive this change.

NilsRodrigues commented 1 year ago
export("==.foo" = equals)

This might help in a small project. I'm thinking of creating an S3 class in a module. There, I need to overload ==, !=, and print. Later, I need to expose these overloads to use the class outside the module. With this solution all code inside the module will need to use the internal function name and can't make use of the overloads.

NilsRodrigues commented 1 year ago
m <- modules::module({
  export(true = !FALSE)
})

I fully understand that nobody likes to make breaking changes. But on the positive side: the fix for this small example would be easy and even shorter than the previous code:

m <- modules::module({
`true` = !FALSE
})
NilsRodrigues commented 1 year ago

Seems like there currently is no simple fix that handles every use case nicely without breaking code.

I haven't made use of any renaming in my small use case and need to get my current code working in a hurry. So for now, I simply made a change to quote everything and use a renamed local copy of the modules package.

wahani commented 1 year ago

@NilsRodrigues can you review https://github.com/wahani/modules/pull/44

You may install that version using

remotes::install_github("wahani/modules", ref = "feat/43-allow-to-export-names-with-special-characters")
NilsRodrigues commented 1 year ago

Thank you for working on the issue. I'm sorry, but this week I don't have time for it. I'll try it next week and come back to you.

wahani commented 1 year ago

Hi @NilsRodrigues any updates on this? Otherwise I would like to close this issue.

NilsRodrigues commented 1 year ago

Moin @wahani, sorry for taking so long. Still swamped. But I did a few tests and it works like a charm. Thank you!

wahani commented 1 year ago

On it's way to CRAN.