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

Closed
jimhester opened this issue May 17, 2018 · 36 comments
Closed

Help collaborate on lintr! #318

jimhester opened this issue May 17, 2018 · 36 comments

Comments

@jimhester
Copy link
Member

@jimhester jimhester 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 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
Copy link
Member Author

@jimhester jimhester 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

Loading

@gdequeiroz
Copy link
Collaborator

@gdequeiroz gdequeiroz commented May 17, 2018

Gabriela de Queiroz

Loading

@MHenderson
Copy link

@MHenderson MHenderson commented May 17, 2018

I would like to help.

Loading

@brankokovac
Copy link

@brankokovac brankokovac commented May 17, 2018

Branko Kovac

Loading

@JamesCuster
Copy link

@JamesCuster JamesCuster commented May 17, 2018

I am interested in helping out.

Loading

@jimhester
Copy link
Member Author

@jimhester jimhester 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

Loading

@erleholgersen
Copy link

@erleholgersen erleholgersen commented May 17, 2018

I'm also interested in helping out.

Loading

@stufield
Copy link
Collaborator

@stufield stufield 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

Loading

@navdeep-G
Copy link

@navdeep-G navdeep-G commented May 18, 2018

I am interested if there is still room. :)

Loading

@adanvr
Copy link

@adanvr adanvr commented May 18, 2018

I would like to help as well

Loading

@dougmet
Copy link
Collaborator

@dougmet dougmet 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)

Loading

@jimhester
Copy link
Member Author

@jimhester jimhester 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.

Loading

@jimhester
Copy link
Member Author

@jimhester jimhester 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!

Loading

@wligtenberg
Copy link
Collaborator

@wligtenberg wligtenberg commented May 18, 2018

I am interested in helping out as well.

Loading

@randy3k
Copy link
Collaborator

@randy3k randy3k commented May 18, 2018

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

Loading

@hfrick
Copy link
Collaborator

@hfrick hfrick commented May 18, 2018

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

Loading

@chucheria
Copy link
Collaborator

@chucheria chucheria commented May 18, 2018

I want to help too! 🙌

Loading

@owenjonesuob
Copy link
Collaborator

@owenjonesuob owenjonesuob commented May 18, 2018

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

Loading

@chauhraj
Copy link

@chauhraj chauhraj 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.

Loading

@grahamrp
Copy link
Collaborator

@grahamrp grahamrp commented May 18, 2018

I'm also keen to help.

Loading

@jimhester
Copy link
Member Author

@jimhester jimhester commented May 18, 2018

Never too late!

Loading

@dragosmg
Copy link
Collaborator

@dragosmg dragosmg commented May 18, 2018

Same here. I'd be keen to help.

Loading

@cryptomanic
Copy link

@cryptomanic cryptomanic commented May 19, 2018

I would love to contribute.

Loading

@gu-stat
Copy link

@gu-stat gu-stat commented May 23, 2018

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

Loading

@jimhester
Copy link
Member Author

@jimhester jimhester 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

Loading

@gdequeiroz
Copy link
Collaborator

@gdequeiroz gdequeiroz 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)

Loading

@jimhester
Copy link
Member Author

@jimhester jimhester 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.

Loading

@jimhester
Copy link
Member Author

@jimhester jimhester commented Jun 8, 2018

It would be great for someone to review #326

Loading

@jabranham
Copy link
Contributor

@jabranham jabranham 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.

Loading

@russHyde
Copy link
Collaborator

@russHyde russHyde commented Sep 25, 2018

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

Loading

@Neil-Schneider
Copy link

@Neil-Schneider Neil-Schneider commented Jan 9, 2019

Hi Jim, I would love to join the team.

Loading

@mjsteinbaugh
Copy link

@mjsteinbaugh mjsteinbaugh 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).

Loading

@jameslamb
Copy link

@jameslamb jameslamb commented Sep 29, 2019

As far as I could tell from #348 , this is the place to kindly request a new release to CRAN. The discussion on #348 predates v1.0.3, which was released in November 2018.

Could you please prepare another release to CRAN?

I'd like to take advantage of these linters in projects that require building from package managers (not GitHub):

github_only_linters <- list(
    "equals_na" = lintr::equals_na_linter
    , "function_left" = lintr::function_left_parentheses_linter
    , "implicit_integers" = lintr::implicit_integer_linter
    , "paren_brace_linter" = lintr::paren_brace_linter
    , "semiconlon" = lintr::semicolon_terminator_linter
    , "seq" = lintr::seq_linter
    , "todo_comments" = lintr::todo_comment_linter
    , "true_false" = lintr::T_and_F_symbol_linter
)

Thanks very much!

Loading

@joaopmatias
Copy link

@joaopmatias joaopmatias commented Dec 15, 2019

Hi, I'd like to contribute, too!

Loading

@renkun-ken
Copy link
Collaborator

@renkun-ken renkun-ken commented Jun 24, 2020

I'd like to contribute more, thanks!

Loading

@MichaelChirico
Copy link
Collaborator

@MichaelChirico MichaelChirico commented Jan 31, 2021

I think this is safe to close; feel free to re-open

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet