coding style #18

deflaux · 2014-10-30T18:11:18Z

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 · 2014-10-30T19:33:18Z

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 · 2014-10-30T21:45:33Z

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

ttriche · 2014-10-30T21:55:27Z

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 · 2014-10-31T01:43:59Z

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 · 2014-10-31T02:17:07Z

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 · 2014-10-31T02:32:48Z

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 · 2014-10-31T03:00:56Z

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 · 2014-10-31T03:05:28Z

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 · 2014-10-31T03:17:59Z

Tim, that makes sense.

Dion, really appreciate it :)

dionloy · 2014-10-31T22:42:44Z

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 · 2014-10-31T23:13:02Z

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 · 2014-10-31T23:14:47Z

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

Have a great weekend!
~p

ttriche · 2014-11-01T04:53:36Z

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 · 2015-03-24T16:36:30Z

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",
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

pgrosu · 2015-03-24T17:02:05Z

Jim, feel free to do a PR and update it :)

siddharthab · 2015-03-24T17:27:07Z

Thanks. We were running the google internal rlint and I missed running it on the tests directory. Looks like some of these suggestions are genuine and can be a good feedback for the rlint team. I will send an update addressing all the suggestions today.

pgrosu · 2015-03-24T18:53:04Z

Sid, so is the rlint code available now? If so it would be useful to have it released. Based on this link, it still seems there should be something there:

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

Thanks,
Paul

siddharthab · 2015-03-24T19:05:41Z

Hi Paul, rlint is still not ready for public release as far as I know. Usually these projects are worked upon as side projects and so it gets very difficult to commit to hard deadlines sometimes.

pgrosu · 2015-03-24T19:13:28Z

Hi Sid,

No problem, that's understandable. In any case, when it does get released we can help out with it :)

Thanks,
~p

deflaux added the enhancement label Oct 30, 2014

deflaux mentioned this issue Jan 15, 2015

submit this package to BioConductor #28

Closed

siddharthab mentioned this issue Mar 24, 2015

With feedback from jimhester's lintr. #33

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coding style #18

coding style #18

deflaux commented Oct 30, 2014

ttriche commented Oct 30, 2014

pgrosu commented Oct 30, 2014

ttriche commented Oct 30, 2014

pgrosu commented Oct 31, 2014

ttriche commented Oct 31, 2014

pgrosu commented Oct 31, 2014

ttriche commented Oct 31, 2014

dionloy commented Oct 31, 2014

pgrosu commented Oct 31, 2014

dionloy commented Oct 31, 2014

ttriche commented Oct 31, 2014

pgrosu commented Oct 31, 2014

ttriche commented Nov 1, 2014

jimhester commented Mar 24, 2015

pgrosu commented Mar 24, 2015

siddharthab commented Mar 24, 2015

pgrosu commented Mar 24, 2015

siddharthab commented Mar 24, 2015

pgrosu commented Mar 24, 2015

coding style #18

coding style #18

Comments

deflaux commented Oct 30, 2014

ttriche commented Oct 30, 2014

pgrosu commented Oct 30, 2014

ttriche commented Oct 30, 2014

pgrosu commented Oct 31, 2014

ttriche commented Oct 31, 2014

pgrosu commented Oct 31, 2014

ttriche commented Oct 31, 2014

dionloy commented Oct 31, 2014

pgrosu commented Oct 31, 2014

dionloy commented Oct 31, 2014

ttriche commented Oct 31, 2014

pgrosu commented Oct 31, 2014

ttriche commented Nov 1, 2014

jimhester commented Mar 24, 2015

pgrosu commented Mar 24, 2015

siddharthab commented Mar 24, 2015

pgrosu commented Mar 24, 2015

siddharthab commented Mar 24, 2015

pgrosu commented Mar 24, 2015