search

CIP-RIU / brapi

An R package to use the Breeding API (BrAPI) for accessing plant breeding data.
13 stars 7 forks source link

Correcting functions to include the formal arguments #10

Closed mverouden closed 7 years ago

mverouden commented 7 years ago

Hi Reinhard,

Perhaps you noticed it already, but I am reviewing your written functions. I have noticed that you most of the time match arguments by position. I would propose to write function arguments in full and match them by name and position. This again to improve readability of the code and to understand what arguments are being passed when using them inside other functions.

Another observation is that you have your indentation set to 4 spaces. I propose to reduce it to 2 spaces, a setting commonly advised in books about R package development (e.g. http://r-pkgs.had.co.nz/r.html section Indentation).

Let me know what you think of these changes.

Regards,

c5sire commented 7 years ago

Hi Maikel,

thanks for this! Indeed, I am all for improving readability - I'll apply both your suggestions in the next cycle of revisions. As well as revising your other suggestions.

Best,

Reinhard

On Fri, May 19, 2017 at 11:06 AM, Maikel Verouden notifications@github.com wrote:

Hi Reinhard,

Perhaps you noticed it already, but I am reviewing your written functions. I have noticed that you most of the time match arguments by position. I would propose to write function arguments in full and match them by name and position. This again to improve readability of the code and to understand what arguments are being passed when using them inside other functions.

Another observation is that you have your indentation set to 4 spaces. I propose to reduce it to 2 spaces, a setting commonly advised in books about R package development (e.g. http://r-pkgs.had.co.nz/r.html section Indentation).

Let me know what you think of these changes.

Regards,

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/c5sire/brapi/issues/10, or mute the thread https://github.com/notifications/unsubscribe-auth/AAafSj9jFIa8FVmaIzvwlSclIwH_cRxVks5r7b3pgaJpZM4Ngt5t .

mverouden commented 7 years ago

If it is ok with you, I will continue my revisions as well. In that case you don't have to rewrite it all by yourself. I am here to help you out.

Was a good meeting on wednesday as well. Hope that the implementation of BrAPI in BMS will be more structured from now on. That would also mean a real opportunity to test BrAPI for R on a extended multi-crop system.

I have also elaborated on the work you have put into the BrAPI for R package in a meeting with VSNi and James Hutton Institute last monday and tuesday in Wageningen. They all recognise the usefulness of your package.

c5sire commented 7 years ago

That is really appreciated! Thanks for passing on the compliments!

I will then leave the changes for the returned tables until you are done. Meanwhile, next week, I will work on finalizing the test data fixture in the brapiTS package.

Have a nice weekend,

Reinhard

On Fri, May 19, 2017 at 2:39 PM, Maikel Verouden notifications@github.com wrote:

If it is ok with you, I will continue my revisions as well. In that case you don't have to rewrite it all by yourself. I am here to help you out.

Was a good meeting on wednesday as well. Hope that the implementation of BrAPI in BMS will be more structured from now on. That would also mean a real opportunity to test BrAPI for R on a extended multi-crop system.

I have also elaborated on the work you have put into the BrAPI for R package in a meeting with VSNi and James Hutton Institute last monday and tuesday in Wageningen. They all recognise the usefulness of your package.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/c5sire/brapi/issues/10#issuecomment-302793372, or mute the thread https://github.com/notifications/unsubscribe-auth/AAafShz-F8W9k7zPTKKezUGjmyvHjUo3ks5r7e_kgaJpZM4Ngt5t .