Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Creates the R-package structure with Best practices #3

Merged
merged 6 commits into from
Aug 5, 2020

Conversation

sk-sahu
Copy link
Contributor

@sk-sahu sk-sahu commented Aug 3, 2020

PR purpose

This creates R-package structure from scratch with usethis pkg best practices.

Adds

In addition to basic structure of R-package it adds

Note

Please ignore current existing functions. Those are old. This PR is just for the structure of R-package. Functions will be rewritten with best practices.

Thanks for the suggestions @cgpu :)

@sk-sahu sk-sahu requested a review from cgpu August 3, 2020 12:10
@sk-sahu sk-sahu linked an issue Aug 3, 2020 that may be closed by this pull request
R/getCohorts.R Outdated Show resolved Hide resolved
Copy link
Contributor

@cgpu cgpu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sk-sahu. Are we addressing the function names and variable names convention to snake_case in this PR as well?

Here's a recap of what we have addressed thus far based on suggestions here:

  • usethis::create_package was used to create the package structure (bonus: out of the box packagedown)
  • Rename repo to lowercase only, no dashes
  • lintr action added in this branch as well from b8909d3
  • for functions and variable names it would be better to use snake_case instead of camelCase
    this is suggested by many but here's a snippet from the Advanced R book.)

Variable and function names should be lowercase. Use an underscore () to separate words within a name. Generally, variable names should be nouns and function names should be verbs.

@sk-sahu
Copy link
Contributor Author

sk-sahu commented Aug 4, 2020

Thank you for the review @cgpu

As I mentioned in the Note section of this PR description. We will adhere the snake_case for function and variable naming. Currently having function are old (written by someone else earlier) and they will be refactored. Working on that in a different branch, it will be a different PR. This PR is mostly for the skeleton of the R-package.

Hope this clears the current concern.

@sk-sahu sk-sahu requested a review from cgpu August 4, 2020 13:42
@sk-sahu
Copy link
Contributor Author

sk-sahu commented Aug 5, 2020

Hi @cgpu

The new commits implements two API end points, although they are in experimental phase (tested in an abstract manner).

And this complies with

  • For functions and variable names it would be better to use snake_case instead of camelCase

Let me know if any other improvements you can suggest with this PR

R/participants_export.R Outdated Show resolved Hide resolved
R/participants_export.R Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
DESCRIPTION Outdated Show resolved Hide resolved
@@ -1,33 +0,0 @@
#' @title Cohort List Extractor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only function not replaced, I assume this is intended as it is depricated? Can you verify @sk-sahu. If this needs to be refactored feel free to open an issue to keep track of what's being re-implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this. Working at the moment. But I'll cross check again.

Also, I'l create an issue to track all up-to-date end points.

Copy link
Contributor

@cgpu cgpu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, can you check the comments and then we should be good to merge this first iteration.

@sk-sahu sk-sahu requested a review from cgpu August 5, 2020 18:20
Copy link
Contributor

@cgpu cgpu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome job @sk-sahu !

@sk-sahu sk-sahu merged commit f913ab0 into master Aug 5, 2020
@sk-sahu sk-sahu deleted the rlib-best-practices branch August 5, 2020 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename repo to cloudos
2 participants