coding style

[deflaux]

There are some competing style guides for R. Let's decide upon which one to follow or some combination of both and make sure the R package version of the client conforms to the style. (see also #17)

http://bioconductor.org/developers/how-to/coding-style/
https://google-styleguide.googlecode.com/svn/trunk/Rguide.xml

Note that the identifier naming convention of avg.clicks in the Google style guide coincides with the S3 methods naming convention and sometimes causes issues/confusion.

http://stackoverflow.com/questions/6583265/what-does-s3-methods-mean-in-r

[ttriche]

Why not just use Google's R style checker?

https://code.google.com/p/google-rlint/

Oh, right... ;-)

Statistics is the grammar of science.
Karl Pearson http://en.wikipedia.org/wiki/The_Grammar_of_Science

On Thu, Oct 30, 2014 at 11:11 AM, Nicole Deflaux notifications@github.com
wrote:

There are some competing style guides for R. Let's decide upon which one
to follow or some combination of both and make sure the R package version
of the client conforms to the style. (see also #17
#17)

http://bioconductor.org/developers/how-to/coding-style/
https://google-styleguide.googlecode.com/svn/trunk/Rguide.xml

Note that the identifier naming convention of avg.clicks in the Google
style guide coincides with the S3 methods naming convention and sometimes
causes issues/confusion.

http://stackoverflow.com/questions/6583265/what-does-s3-methods-mean-in-r

—
Reply to this email directly or view it on GitHub
#18.

[pgrosu]

Thanks Tim - that's very nice and helpful! :)

[ttriche]

Not really. My point was that it's been "in review" for over 3 months
post-useR. Not cool, especially since it was announced to hundreds of
attendees as coming Real Soon Now.

Hoping to poke that with a stick (namely a "useful to public-facing GOOG
projects" stick). It would, in fact, be tremendously useful and widely used
if the code were available.

Statistics is the grammar of science.
Karl Pearson http://en.wikipedia.org/wiki/The_Grammar_of_Science

On Thu, Oct 30, 2014 at 2:45 PM, Paul Grosu notifications@github.com
wrote:

Thanks Tim - that's very nice and helpful! :)

—
Reply to this email directly or view it on GitHub
#18 (comment)
.

[pgrosu]

I know what you mean, but I would say the positive greatly outweighs things in this situation - so nothing really to worry about :) It's a fantastic opportunity for both codebases to grow together. Some of the best experiences I had in bringing new features to users, was when diverse teams connected with each other in the early stages of their respective projects and helped each other out. I always get excited when folks ask me, "Hey do you still have that cool example you showed me a while ago..." - I still get surprised how often they usually turn into a win-win for everybody.

~p

[ttriche]

The problem with the rlint project is simple: there is no codebase.

https://code.google.com/p/google-rlint/source/browse/#svn%2Ftrunk%253Fstate%253Dclosed

I'd be delighted to patch any bugs but I'm not going to write something from scratch that a team of googlers announced publicly to hundreds of people. It's not going to grow if nobody puts up the code.

Either the presenters took a spot at useR which we could have given to someone else, or nobody ever followed through. Every time this issue of R style comes up, it reminds me of the conversation I had with the presenters in July. I just want them to do what they said they would. If it looks like crap, so what? I'll send patches. No code = no patches.

I hope you're right and this provides the impetus for a useful codebase to come out of hibernation. I simply can't grasp why four months isn't long enough to review a python script for release.

Just grep out the obscenities and commit it. I've certainly done that often enough.

[pgrosu]

Aha, now I got it. Looks like Andy Chen (andych@google.com) is the corresponding author on the following presentation:

http://research.google.com/pubs/archive/42577.pdf

I'd be happy to send an email to get an update, unless you prefer to.

Thanks,
~p

[ttriche]

Andy presented it, but the fellow I was talking to was handling the code review. I guess the sensible thing is for me to figure out the latter, contact both, and say "hey, we've got a public-facing use case for an R style enforcer, and it would be great not to reinvent the wheel. Can you please put the google-rlint code up, even if you have to use a pseudonym?"

Would any of the Googlers here prefer to do the honors? When I was there a million years ago, Google was pretty small and everybody knew everybody. I guess that's not the case so much anymore, but still.

It's such an obvious use case.

--t

On Oct 30, 2014, at 7:32 PM, Paul Grosu notifications@github.com wrote:

Aha, now I got it. Looks like Andy Chen (andych@google.com) is the corresponding author on the following presentation:

http://research.google.com/pubs/archive/42577.pdf

I'd be happy to send an email to get an update, unless you prefer to.

Thanks,
~p

—
Reply to this email directly or view it on GitHub.

[dionloy]

Tim, Paul, let me reach out to the folks involved in that code. There looks
to be some recent activity in it internally so it does still look active.

On Thu, Oct 30, 2014 at 8:00 PM, Tim Triche, Jr. notifications@github.com
wrote:

Andy presented it, but the fellow I was talking to was handling the code
review. I guess the sensible thing is for me to figure out the latter,
contact both, and say "hey, we've got a public-facing use case for an R
style enforcer, and it would be great not to reinvent the wheel. Can you
please put the google-rlint code up, even if you have to use a pseudonym?"

Would any of the Googlers here prefer to do the honors? When I was there a
million years ago, Google was pretty small and everybody knew everybody. I
guess that's not the case so much anymore, but still.

It's such an obvious use case.

--t

On Oct 30, 2014, at 7:32 PM, Paul Grosu notifications@github.com
wrote:

Aha, now I got it. Looks like Andy Chen (andych@google.com) is the
corresponding author on the following presentation:

http://research.google.com/pubs/archive/42577.pdf

I'd be happy to send an email to get an update, unless you prefer to.

Thanks,
~p

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub
#18 (comment)
.

[pgrosu]

Tim, that makes sense.

Dion, really appreciate it :)

[dionloy]

I've heard back from the engineers responsible. I don't want to make any
promises, but things are moving.

On Thu Oct 30 2014 at 8:18:03 PM Paul Grosu notifications@github.com
wrote:

Tim, that makes sense.

Dion, really appreciate it :)

—
Reply to this email directly or view it on GitHub
#18 (comment)
.

[ttriche]

Awesome. Thanks Dion!

--t

On Oct 31, 2014, at 3:42 PM, Dion Loy notifications@github.com wrote:

I've heard back from the engineers responsible. I don't want to make any
promises, but things are moving.

On Thu Oct 30 2014 at 8:18:03 PM Paul Grosu notifications@github.com
wrote:

Tim, that makes sense.

Dion, really appreciate it :)

—
Reply to this email directly or view it on GitHub
#18 (comment)
.

—
Reply to this email directly or view it on GitHub.

[pgrosu]

Thanks Dion - please keep us in the loop when it becomes available.

Have a great weekend!
~p

[ttriche]

Nb. If they're really slammed for time, there's now a reasonable alternative in https://github.com/jimhester/lintr. This works for Hadley so it's another option if the google project gets stuck

Not passing judgment one way or the other, just noting recent development.

--t

On Oct 31, 2014, at 3:42 PM, Dion Loy notifications@github.com wrote:

I've heard back from the engineers responsible. I don't want to make any
promises, but things are moving.

On Thu Oct 30 2014 at 8:18:03 PM Paul Grosu notifications@github.com
wrote:

Tim, that makes sense.

Dion, really appreciate it :)

—
Reply to this email directly or view it on GitHub
#18 (comment)
.

—
Reply to this email directly or view it on GitHub.

[jimhester]

FWIW running lintr on the project with lintr::lint_package(linters=with_defaults(camel_case_linter = NULL)) yields the following lints.

R/client.R:88:12: style: Use <-, not =, for assignment.

᠎  messages = list()
           ^

R/client.R:121:38: style: Only use double-quotes.

᠎    warning(paste(messages, collapse='\n'), immediate.=TRUE)
                                     ^~~~

R/reads.R:117:1: style: lines should not be more than 80 characters.

᠎    # TODO improve performance https://github.com/googlegenomics/api-client-r/issues/17
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

R/reads.R:147:57: style: Closing curly-braces should always be on their own line, unless it's followed by an else.

᠎                   cigar_enum_map[cigarPiece$operation])}),
                                                        ^

R/reads.R:224:67: style: Only use double-quotes.

᠎readsToGAlignments <- function(reads, oneBasedCoord=TRUE, slStyle='UCSC') {
                                                                  ^~~~~~

R/reads.R:231:26: style: Only use double-quotes.

᠎  names <- sapply(reads, '[[', 'fragmentName')
                         ^~~~

R/reads.R:231:32: style: Only use double-quotes.

᠎  names <- sapply(reads, '[[', 'fragmentName')
                               ^~~~~~~~~~~~~~

R/reads.R:242:3: warning: local variable ‘total_reads’ assigned but may not be used

᠎  total_reads <- length(positions)
  ^~~~~~~~~~~

R/reads.R:246:53: style: Only use double-quotes.

᠎      strand=strand(as.vector(ifelse(isMinusStrand, '-', '+'))),
                                                    ^~~

R/reads.R:246:58: style: Only use double-quotes.

᠎      strand=strand(as.vector(ifelse(isMinusStrand, '-', '+'))),
                                                         ^~~

R/variants.R:110:1: style: lines should not be more than 80 characters.

᠎    # TODO improve performance https://github.com/googlegenomics/api-client-r/issues/17
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

R/variants.R:146:69: style: Only use double-quotes.

᠎variantsToVRanges <- function(variants, oneBasedCoord=TRUE, slStyle='UCSC') {
                                                                    ^~~~~~

R/variants.R:199:69: style: Only use double-quotes.

᠎variantsToGRanges <- function(variants, oneBasedCoord=TRUE, slStyle='UCSC') {
                                                                    ^~~~~~

tests/runTests.R:6:36: style: Put spaces around all infix operators.

᠎if (!is.na(apiKey) && nchar(apiKey)>0) {
                                  ~^~

tests/testthat/test-getVariants.R:3:1: style: Use two spaces to indent, never tabs.

᠎             start=50300077, end=50301500, ...)
^

tests/testthat/test-getVariants.R:16:1: style: lines should not be more than 80 characters.

᠎                                       "referenceBases", "alternateBases", "quality",
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~