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

Help collaborate on lintr! #318

Open
jimhester opened this issue May 17, 2018 · 32 comments

Comments

@jimhester
Copy link
Owner

commented May 17, 2018

As announced at https://twitter.com/jimhester_/status/997109466674819074 I am interested in having some help working on lintr. This issue can serve as a place for those interested to 'sign up' and also to discuss what priorities. So reply below if you are interested and I will invite you to be a collaborator on the project.

@jimhester jimhester changed the title Help collaborate on lintr Help collaborate on lintr! May 17, 2018

@jimhester

This comment has been minimized.

Copy link
Owner Author

commented May 17, 2018

How (I hope) this will work

Everyone who replies to this issue will be invited to be collaborators on the lintr project.

What exactly happens if I accept? Does it mean I’ll break everything if I click the wrong button?

Concretely, if you accept the invitation, this does these things:

  • It lets you manage incoming issues on by labelling them, closing them, etc.
  • It lets you merge pull requests by clicking Github’s big green “Merge” button, but only if all their tests have passed.
  • It automatically subscribes you to notifications (but you can unsubscribe again if you want through the Github interface)
  • It does not allow you to push changes directly to Github without submitting a PR, and it doesn’t let you merge broken PRs – this is enforced through Github’s “branch protection” feature, and it applies to everyone from the newest contributor up to the project founder.

Okay, that’s what I CAN do, but what SHOULD I do?

Short answer: whatever you feel comfortable with.

We do have one rule, which is the same one most F/OSS projects use: don’t merge your own PRs. We find that having another person look at each PR leads to better quality. (Exception: you may see @jimhester merging his own PRs. This happens because he is lonely and has no-one to review them for him. It would make him happy if you reviewed and – if they look good – merged his PRs.)

Beyond that, it all comes down to what you feel up to. If you don’t feel like you know enough to review a complex code change, then you don’t have to – you can just look it over and make some comments, even if you don’t feel up to making the final merge/no-merge decision. Or you can just stick to merging doc fixes and adding tags to issues, that’s very helpful too. If after hanging around for a while you start to feel like you have better handle on how things work and want to start doing more, that’s excellent; if it doesn’t happen, that’s fine too.

If at any point you’re unsure about whether doing something would be appropriate, feel free to ask (by posting Github comment). For example, it’s totally OK if the first time you review a PR, you want someone else to check over your work before you hit the merge button.

(adapted from https://trio.readthedocs.io/en/latest/contributing.html#joining-the-team)

Tasks to perform

  • - Triage all open issues, using the tidyverse labels. Each issue should have at least one label.
  • - Review open pull requests, seeing if they can be adapted or should just be closed.
  • - Assess performance of the devel version vs that of the current CRAN version (devel is quite a bit slower)
  • - Get devel version in a state where we can have a new CRAN release.
  • - Consider feasibility of converting existing linters to using XPath expressions rather than R code (this is advanced, but could make things much faster) #236 is an example
@gdequeiroz

This comment has been minimized.

Copy link
Collaborator

commented May 17, 2018

Gabriela de Queiroz

@MHenderson

This comment has been minimized.

Copy link
Collaborator

commented May 17, 2018

I would like to help.

@brankokovac

This comment has been minimized.

Copy link
Collaborator

commented May 17, 2018

Branko Kovac

@JamesCuster

This comment has been minimized.

Copy link
Collaborator

commented May 17, 2018

I am interested in helping out.

@jimhester

This comment has been minimized.

Copy link
Owner Author

commented May 17, 2018

So do you all think it would be helpful to have a gitter chat room so you could have more real time access for questions etc, or are GitHub comments enough?

Ok added one https://gitter.im/jimhester-lintr/Lobby

@erleholgersen

This comment has been minimized.

Copy link
Collaborator

commented May 17, 2018

I'm also interested in helping out.

@stufield

This comment has been minimized.

Copy link
Collaborator

commented May 17, 2018

Hi Jim,
Love what you've started with lintr and would be happy to be involved to the extent I can contribute.
Stu

@navdeep-G

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2018

I am interested if there is still room. :)

@adanvr

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2018

I would like to help as well

@dougmet

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2018

Hi, a few of us from Mango might be able to muck in as it slots together quite well with mangothecat/goodpractice and friends. cc @hfrick @sellorm @owenjonesuob

We use lintr heavily on lots of packages as part of ValidR (via mangothecat/packageMetrics2)

@jimhester

This comment has been minimized.

Copy link
Owner Author

commented May 18, 2018

@dougmet, that would be great, If the other Mango folks are interested please add a separate reply and I can add you to the collaborators.

@jimhester

This comment has been minimized.

Copy link
Owner Author

commented May 18, 2018

Also most of the linters in goodpractice that would be great to have in lintr, so a good first contribution would be doing that!

@wligtenberg

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2018

I am interested in helping out as well.

@randy3k

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2018

I am the author of https://github.com/REditorSupport/languageserver
I’d also like to be a collaborator.

@hfrick

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2018

Hi @jimhester, some lintr/goodpractice crossover would be nice. 👋

@chucheria

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2018

I want to help too! 🙌

@owenjonesuob

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2018

Hiya, great to see interest in lintr-goodpractice collaboration! I'm definitely keen to help out.

@chauhraj

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2018

I am late to the party but would like to help too. I hope I am not too late to join.

@grahamrp

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2018

I'm also keen to help.

@jimhester

This comment has been minimized.

Copy link
Owner Author

commented May 18, 2018

Never too late!

@dragosmg

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2018

Same here. I'd be keen to help.

@cryptomanic

This comment has been minimized.

Copy link
Collaborator

commented May 19, 2018

I would love to contribute.

@gu-stat

This comment has been minimized.

Copy link
Collaborator

commented May 23, 2018

I'd like to help, if there's still room.

@jimhester

This comment has been minimized.

Copy link
Owner Author

commented Jun 3, 2018

A great first issue PR for someone to review would be #321, it mainly needs a note added to NEWS.md

@gdequeiroz

This comment has been minimized.

Copy link
Collaborator

commented Jun 6, 2018

Hi @jimhester, what do you mean by "assess performance" in Assess performance of the devel version vs that of the current CRAN version (devel is quite a bit slower)

@jimhester

This comment has been minimized.

Copy link
Owner Author

commented Jun 6, 2018

Measure how badly the performance has degraded and if possible which linters are most responsible for the degradation.

Ideally with something like profvis but as a start even using system.time() with a large R file that would have many lints, check.R in the tools package is an extreme case.

download.file("https://raw.githubusercontent.com/wch/r-source/3ddb337d62817599d3cb739c8815bf2276e01511/src/library/tools/R/check.R", "check.R")
system.time(lints <- lintr::lint("check.R", cache = FALSE))

On my machine this takes ~60-70 seconds with the current CRAN version, the devel version takes about 9 minutes.

So you will likely need to use a smaller file to narrow down the exact causes, but good first step is identifying the extent of the problem.

@jimhester

This comment has been minimized.

Copy link
Owner Author

commented Jun 8, 2018

It would be great for someone to review #326

@jabranham

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2018

We can measure the speed difference between various linters by looping over the defaults:

library(lintr)
x <- list(length = length(names(lintr::default_linters)))
i <- 1
for (name in lintr::default_linters){
  x[[i]] <- system.time(
    lint("check.R",
         linters = name,
         cache = FALSE))
  names(x)[i] <- names(lintr::default_linters[i])
  i <- i + 1
}

comparing the CRAN and github versions I get the following speeds:

                                   CRAN      GH
assignment_linter                 6.796   7.905
single_quotes_linter              7.633   8.772
absolute_paths_linter             9.637      NA
no_tab_linter                     8.007   9.637
line_length_linter                6.743   7.937
commas_linter                     8.745  10.155
infix_spaces_linter               8.687   9.975
spaces_left_parentheses_linter    9.251  10.608
spaces_inside_linter              7.862   9.471
open_curly_linter                 7.107   8.612
closed_curly_linter               7.077   8.572
camel_case_linter                33.693      NA
multiple_dots_linter              9.081      NA
object_length_linter              8.342 196.771
object_usage_linter              11.742  13.594
trailing_whitespace_linter        8.132   9.175
trailing_blank_lines_linter       6.588   7.903
commented_code_linter             8.349   8.495
function_left_parentheses_linter     NA  10.796
object_name_linter                   NA 196.275
pipe_continuation_linter             NA  75.777

So it looks to me that the three linters responsible for the slowdown are object_name_linter, object_length_linter, and pipe_continuation_linter. Those could just be removed from the defaults, if a CRAN release is planned soon.

@russHyde

This comment has been minimized.

Copy link
Collaborator

commented Sep 25, 2018

Happy to pitch-in if you still need helpers.
Russ Hyde, Glasgow Uni.

@Neil-Schneider

This comment has been minimized.

Copy link

commented Jan 9, 2019

Hi Jim, I would love to join the team.

@mjsteinbaugh

This comment has been minimized.

Copy link

commented Jan 9, 2019

@jimhester I'm up for adding some linters that help adhere to the Bioconductor coding style.

Also, is there an easy way to get S3 methods to pass linter checks (e.g. function.default will warn about the period).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.