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

change import function in R #39

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

sbenateau
Copy link

Use data.table::fread, a more versatile function to import data. Allows the use of space, comma, semicolon as separator. Instead of only space and comma

Same as the one I cancelled but probably cleaner...

@sbenateau
Copy link
Author

Ok the Lint R script is quite impressive but it would require a lot of changes...

@yvanlebras
Copy link
Contributor

Thank you Simon! Don't hesitate to ask if we can help somewhere! Here we updated the CI workflow from Travis system to Github actions, that's why tests are different, notably considering the lint R part who allows to aply best practices to code. I don't know why the "Combine chunked test results" is failing regarding tool testing....

@sbenateau
Copy link
Author

Do you have access to this file? 'All tool test results' artifact for details.
I won't be able to make all the changes to pass the lint R test:

  • Variable and function name style should be snake_case -> this can be tricky as column names are sometimes names thank to this and therefore it require a lot of time to make sure it's ok
  • functions should have cyclomatic complexity of less than 15, this has 28. -> this require to rewrite a function I don't fully understand and I don't think I have the skills and / or enough time to do that.

@bgruening
Copy link
Collaborator

@sbenateau please have a look at https://rdrr.io/cran/lintr/man/exclude.html

@sbenateau
Copy link
Author

Thank you @bgruening this is what we need. @yvanlebras where can I edit the lint config file ?

@sbenateau
Copy link
Author

I now have some tests failing... I must have done something wrong with all this reformatting.

@bgruening
Copy link
Collaborator

Oh you can add # nolint to your code line. And then it will be skipped.

@sbenateau
Copy link
Author

sbenateau commented Jan 12, 2021

all tests passed again in planemo :-). I will try your trick @bgruening, thank you.

@sbenateau
Copy link
Author

Any ideas? The reformatting of the R script seems to be ok planemo gives me no error. But I don't get the last error.

@bgruening
Copy link
Collaborator

image

:)

@sbenateau
Copy link
Author

It looks like an encoding problem... It works on my computer.

@yvanlebras
Copy link
Contributor

Yes, encoding problem apparently looking at the "..residuals.." sentences

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.

3 participants