Refactoring - Githubissues

sonatype-nexus-community / oysteR

Create purls from the filtered sands of your dependencies, powered by OSS Index

https://sonatype-nexus-community.github.io/oysteR/

Apache License 2.0

40 stars 9 forks source link

Refactoring #3

Closed csgillespie closed 4 years ago

csgillespie commented 4 years ago

Hi. I saw your message on the R-pkg mailing list and thought your package looked interested and useful. So I wondered if I could solve your issue. However, I found a few other issues, some of which would have resulted in your package being bounced by CRAN.

The issues I found (and fixed) were

Manipulating the namespace path via library commands
Running functions on loading
Exporting all functions
Using "." for function names, but then exporting them.

I also fixed

Made some of the functions more R like
Returned a data.frame/tibble for easier manipulation by users

On loading the package you can now:

library(oysteR)
results = audit_deps()
get_vulnerabilities(results)

This PR passes all CRAN checks. I'm probably going to start using this package in a bit more anger in the next few days.

I realise that this is a ridiculously long, unasked for and not discussed PR. So if you want to close that's fine. Happy to discuss and contribute further.

DarthHater commented 4 years ago

This is AWESOME!!!! Thank you so much for popping in! We are all relatively newbies working with R, so we love that we can learn from you and the rest of the community. We will likely take a look and merge this (or ask for some changes) on Thursday!

csgillespie commented 4 years ago

Made a few more tweaks. The main one is being able to pass in a data frame of packages, instead of relying on installed.packages()

A few minor points:

There's no test suite
My R packages didn't have many vulnerabilities, so get_vulnerabilities isn't well tested
In the R world, travis is well support. I'm happy to add a quick .travis file that tests under different R version. Likely on ~6 lines long
Happy to be a co-author (not really bothered), but it might encourage me to answer future queries

DarthHater commented 4 years ago

@csgillespie that all sounds great! Yeah I only know of a few R packages with known vulnerabilities myself, but I'd imagine there's quite a bit more lurking (the use of C makes me imagine that, can't spell CVE without C!).

I'll try and take a gander today, but tomorrow at the latest. Biggest things I can think of so far is make sure to add a license header to new files.

DarthHater commented 4 years ago

Gave stuff a cursory look, tomorrow @brittanybelle @adrianpowell or myself will give it a better look I imagine!

I noticed the Development section is missing now. I was mostly curious about that as it seems you are using R Studio or something akin, we have all been kinda cranking on this with Sublime/VS Code and using R via the command line.

csgillespie commented 4 years ago

I noticed the Development section is missing now. I was mostly curious about that as it seems you are using R Studio or something akin, we have all been kinda cranking on this with Sublime/VS Code and using R via the command line.

So your development section wasn't really correct :( In general, you would never run a single file in an R package Rscript -e R/main.R as (is the case is now), we have functions in separate files. Happy to have a video chat and give an overview of RStudio etc.

DarthHater commented 4 years ago

Hah, learning all kinds of things! The extent of my R experience was more or less finishing a Coursera course, not really heavy on the real life R use :)

I think all three of us might have some time tomorrow! Would love to catch up and get some good pointers, etc...

We are really stoked to see how engaged you are with this. It's fun to give back to communities, and hopefully this tool can be helpful to R developers in general!

csgillespie commented 4 years ago

@DarthHater What about creating a new branch then we can merge straightaway and tackle issues one-by-one. I suspect this thread will get messy!

DarthHater commented 4 years ago

@csgillespie for sure. Maybe a PR per thing you would like to get merged? That's generally how we go about it.

csgillespie commented 4 years ago

@DarthHater Yep (I realise this massive PR isn't the usual way to go). So I'm thinking:

New branch on this repo - dev perhaps?
Automatically merge this PR to dev
Then create new single issues on dev to work through.
Once all issues are down, merge to master?

DarthHater commented 4 years ago

Created a dev branch for you! I'll invite you to the @csgillespie org too, so you can work on this project a bit better :)

DarthHater commented 4 years ago

Merged to dev, I've sent you an invite as a collaborator/sonatype-nexus-community member, you should be able to work directly on that branch now :)

csgillespie commented 4 years ago

@DarthHater I've pulled out your unresolved comments and created corresponding issues.