Conversion of functions to S4 in FTP download part of package

[julien-roux]

(Originally posted by @wirawara): The download part of package should be re-written in S4 system.

(Added by me): Is this necessary? Why would it be better (apart from more elegant code)?

[wirawara]

I thought I said why would it be better. S3 functions are not stable, while S4 are. The main issue is the class inheritance which is messed up in S3. The code will be more stable, and well written. http://stackoverflow.com/questions/6450803/class-in-r-s3-vs-s4 http://adv-r.had.co.nz/S4.html

[fbastian]

Well, the stackoverflow answer seems to say "depends on your project, I never needed S4 so far"

[wirawara]

@fbastian You can check in which form is GEOquery written. I don't see why is such an issue to make code better. After all, this is a software and not some research project where you just want to make your code to work. I am fine with S3 there. Just find S3 a bit amateur for a package.

[julien-roux]

A few comments after checking these links:

• There is no "stability" issue with S3. It is the simplest, most compact way of dealing with objects. Of course since it is simple, it is a less rigorous, and less powerful than other class types. Anyway this does not concern us, since we do not use S3 classes in the package (check again http://adv-r.had.co.nz/OO-essentials.html :p)
• In Bgee.R we use a Reference class, which is declared with the SetRefClass() function. This system is easier to people familiar with other object-oriented languages: http://adv-r.had.co.nz/OO-essentials.html#picking-a-system
• In topAnat.R we use a S4 class, since we extend the topGOdata S4 class from the topGO package. The topAnatData class is declared with the setClass() function.

So the question here is: should we switch everything to the S4 class system? This would be a bit more elegant, but it will take some work, can introduce bugs, and really there is no issue with the actual code. So my recommendation is to keep things as is in the near future.

=> Let's keep this issue open with a low priority tag.

PS: The GEOquery package choices are not really relevant to this debate, since we do not depend on them

[wirawara]

I know that we are using RefClass in Bgee.R, I wrote it. :P And I know that there is S4 class in topAnat.R, but then Download part is something completely different, which I am complaining on.

Anyways, its good @julien-roux that you put the points. I am for to be re-written either in S4 or RefClasses. Maybe better in S4 since we depend on topGO.

I agree that this is for now low priority, but seems it needed discussion to agree upon. Was not sure why was a such a problem to convince.

ps. choices of GEOquery are an example how does package looks like in S4 classes and it looks good. We are aware that we don't depend on them.

[julien-roux]

I know that we are using RefClass in Bgee.R, I wrote it. :P And I know that there is S4 class in topAnat.R, but then Download part is something completely different, which I am complaining on.

Ok then I am not sure what you are referring to when you mentioned S3 classes and "download part". Could you clarify? Or give a script name and line numbers?

Anyways, its good @julien-roux that you put the points. I am for to be re-written either in S4 or RefClasses. Maybe better in S4 since we depend on topGO.

I agree that this is for now low priority, but seems it needed discussion to agree upon. Was not sure why was a such a problem to convince.

I see your point, but I just think there are a lot of higher priority things to work on

[julien-roux]

See also http://adv-r.had.co.nz/R5.html

It doesn't seem to me that the specificities of RC (mutability) are needed for the Bgee class... In fact (after discussion with Hervé Pagès at the super advanced R course on June 2017) they could also be dangerous, since this is not a behavior that users are expecting. The syntax is also unusual for R users.

So I would now be a bit more in favor of switching the Bgee class to S4...