search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
725 stars 38 forks source link

[REVIEW]: lintr: Static Code Analysis for R #7240

Open editorialbot opened 2 months ago

editorialbot commented 2 months ago

Submitting author: !--author-handle-->@jimhester<!--end-author-handle-- (James Hester) Repository: https://github.com/r-lib/lintr Branch with paper.md (empty if default branch): Version: v3.1.2 Editor: !--editor-->@lrnv<!--end-editor-- Reviewers: @JosiahParry, @SaranjeetKaur Archive: Pending

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/0f4eccbd59ee5bf5a5be88489581970f"><img src="https://joss.theoj.org/papers/0f4eccbd59ee5bf5a5be88489581970f/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/0f4eccbd59ee5bf5a5be88489581970f/status.svg)](https://joss.theoj.org/papers/0f4eccbd59ee5bf5a5be88489581970f)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@JosiahParry & @SaranjeetKaur, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @lrnv know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Checklists

📝 Checklist for @JosiahParry

📝 Checklist for @SaranjeetKaur

editorialbot commented 2 months ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 2 months ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

✅ OK DOIs

- 10.21105/joss.01686 is OK

🟡 SKIP DOIs

- No DOI given, and none found for title: The Tidyverse Style Guide
- No DOI given, and none found for title: R: A Language and Environment for Statistical Comp...
- No DOI given, and none found for title: Code Complete
- No DOI given, and none found for title: Static program analysis — Wikipedia, The Free Ency...

❌ MISSING DOIs

- None

❌ INVALID DOIs

- None
editorialbot commented 2 months ago

Software report:

github.com/AlDanial/cloc v 1.90  T=0.15 s (2445.7 files/s, 308678.0 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
R                              333           4204           8314          28584
XML                              2              0            129           1797
Markdown                        10            360              0           1469
YAML                            13             83             53            433
Rmd                             10            372            881            200
CSV                              1              0              0            124
JSON                             2              0              0             61
vim script                       1             14             21             50
TeX                              1              4              0             37
Dockerfile                       1              3              0             11
-------------------------------------------------------------------------------
SUM:                           374           5040           9398          32766
-------------------------------------------------------------------------------

Commit count by author:

   583  Michael Chirico
   381  Jim Hester
   214  Indrajeet Patil
   155  AshesITR
    88  Alexander Rosenstock
    57  Florent Angly
    46  Kun Ren
    24  Russ Hyde
    13  Kirill Müller
    10  Dragoș Moldovan-Grünfeld
     8  Forest Fang
     8  Hugo Gruson
     8  dependabot[bot]
     7  Gábor Csárdi
     7  MEO265
     7  Michael Quinn
     7  Russell Hyde
     6  Barret Schloerke
     5  Salim B
     4  JhossePaul
     4  Jonathan Keane
     4  jrnold
     3  Bruce Lee
     3  Daniel Possenriede
     3  Fabian Scheipl
     3  Jennifer (Jenny) Bryan
     3  Konrad Pagacz
     3  Laurent Gatto
     3  eitsupi
     3  huisman
     3  nathaneastwood
     3  olivroy
     2  Alex Branham
     2  Ashley Baldry
     2  Chris Black
     2  Dan Kessler
     2  F-Noelle
     2  Iñaki Úcar
     2  Jack Wasey
     2  Konrad Rudolph
     2  Maëlle Salmon
     2  Micah J Waldstein
     2  Rafael Zayas
     2  dmurdoch
     1  Alessandro Gentilini
     1  Alexis Iglauer
     1  Andrew Choi
     1  Andrés Felipe Quintero Moreano
     1  Anton Bossenbroek
     1  Bernie Gray
     1  Brandon Bertelsen
     1  Christian Diener
     1  Colin Rundel
     1  Daniel Sabanes Bove
     1  Dave Lovell
     1  Derek Chiu
     1  Dragos Moldovan-Grunfeld
     1  Ellis Valentiner
     1  Fleur Kelpin
     1  Florian Kohrt
     1  Frans van Dunné
     1  Frédéric Mahé
     1  Gabor Csardi
     1  Gabriela de Queiroz
     1  Guillaume Gaullier
     1  Hadley Wickham
     1  Hannah Frick
     1  Hao Ye
     1  Hedley
     1  Henning Lorenzen
     1  Hiroaki Yutani
     1  JJ Allaire
     1  James Baird
     1  Jamie Owen
     1  Jeffrey Arnold
     1  Jenny Bryan
     1  Jeremy Werner
     1  Jon Harmon
     1  Josh
     1  Kara Woo
     1  Landon Abney
     1  Leonardo Gama
     1  Mara Averick
     1  Marcel Schilling
     1  Marie-Helene Burle
     1  Mark Miller
     1  Matt Brennan
     1  Matthew T. Warkentin
     1  Nic
     1  Nicholas Masel
     1  Paolo Di Lorenzo
     1  Paul Kaefer
     1  Paul Staab
     1  Randy Lai
     1  Shaopeng
     1  StefanBRas
     1  Stu Field
     1  The Gitter Badger
     1  Tony Kenny
     1  Wesley Burr
     1  Will Landau
     1  Yu ISHIKAWA
     1  Yuu ISHIKAWA
     1  arekbee
     1  banky
     1  bernie gray
     1  jeffwong-nflx
     1  jmaspons
     1  ttriche
editorialbot commented 2 months ago

Paper file info:

📄 Wordcount for paper.md is 1498

✅ The paper includes a Statement of need section

editorialbot commented 2 months ago

License info:

🟡 License found: Other (Check here for OSI approval)

lrnv commented 2 months ago

@JosiahParry, @SaranjeetKaur greetings! Thanks for accepting to take a bit of time to review this submission. First, do you know how JOSS review are handled, or do you need me to wrap it up for you ? The first thing you have to do is to generate a guide for you -- formatted as a checklist --, using a command @editorialbot generate my checklist. in this discussion. Then if you have more questions, I'll be happy to help

editorialbot commented 2 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

JosiahParry commented 2 months ago

Review checklist for @JosiahParry

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

JosiahParry commented 2 months ago

The package, documentation, and testing is great. Please address the below regarding the paper. Notably there isn't a clear summary of lintr for a non-specialist audience, nor is there a state of the field section.

Paper

IndrajeetPatil commented 1 month ago

@editorialbot generate pdf

editorialbot commented 1 month ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

IndrajeetPatil commented 1 month ago

@JosiahParry Thanks a lot for your feedback! We have updated the draft to address your concerns. Please let us know if you have any additional feedback!

P.S. We are also currently looking into a possible encoding issue seen in the PDF output:

Screenshot 2024-10-02 at 08 27 11
lrnv commented 1 month ago

@IndrajeetPatil Happens sometimes in Julia when some characters are not availiable in the monospace font used. You can change the default monospace font (for one that includes the glyph you need) by adding:

header-includes:
- |
  ```{=latex}
  \setmonofont[Path=./]{MyFont-Regular.ttf}


to the yaml header of the paper. See https://github.com/openjournals/joss/issues/963 for wide discussions if this does not work.
lrnv commented 1 month ago

Hey @SaranjeetKaur have you had time to take a look at this review ?

SaranjeetKaur commented 1 month ago

Hi @lrnv, I was on leave, so haven't managed to catch up with it. I have it on my to-do, so will get to it soon.

JosiahParry commented 3 weeks ago

This is all good from my side! Good work all! Many years in the making :)

SaranjeetKaur commented 3 weeks ago

Review checklist for @SaranjeetKaur

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

SaranjeetKaur commented 3 weeks ago

Thanks for all your work! Great package and documentation!

Paper

Naturally, we can’t discuss all of them here. To see details about all available linters, we encourage readers to see https://lintr.r- lib.org/dev/reference/index.html#individual- linters.

This could perhaps be rephrased as (instead of putting a link, it could be a hyperlink?):

To see the most up-to-date details about all the available linters, we encourage readers to refer to the list of individual linters.

IndrajeetPatil commented 3 weeks ago

Happy to rephrase that way.

instead of putting a link, it could be a hyperlink?

That's not friendly to readers who like to read the printed versions of publications (me being one of them 😉)

SaranjeetKaur commented 3 weeks ago

That's not friendly to readers who like to read the printed versions of publications (me being one of them 😉)

I see, I didn't realise that! Thanks for clarifying!

SaranjeetKaur commented 3 weeks ago

Although the "Statement of Need" section is giving the required info, it talks about the tool before mentioning why the tool is required. Perhaps a sentence or two, might help clarify? What do you think?

For example,

In computer programming, "linting" is the process of analysing the source code to identify possible programming and stylistic problems (Refer: https://en.wiktionary.org/wiki/linting). This can be done using a software tool called a linter (or a lint tool). A linter analyzes code to identify potential errors, stylistic issues, or deviations from coding standards. This helps ensure consistency, readability, and best practices by flagging common mistakes, such as syntax errors, unused variables, or improper formatting. Linters are essential for improving code quality, preventing bugs, and maintaining a clean codebase, especially in collaborative development environments (Wikipedia contributors, 2024). {lintr} is an open-source package that provides linters for the R programming language, which is an interpreted, dynamically-typed programming language (R Core Team, 2023), and is used by a wide range of researchers and data scientists. {lintr} can thus act as a valuable tool for R users to help improve the quality and reliability of their code.

SaranjeetKaur commented 3 weeks ago

Installing the development version from GitHub (packageVersion("lintr") # ‘3.1.2.9000’) gives length 113:

remotes::install_github("r-lib/lintr")

library(lintr)

length(all_linters())
#> [1] 113

Whereas installing the stable version on CRAN (packageVersion("lintr") # ‘3.1.2’) gives length 96:

install.packages("lintr")

library(lintr)

length(all_linters())
#> [1] 96

Do you think this should be explicitly mentioned, with say, As of this writing, the development version of {lintr} from GitHub offers 113 linters? Followed by,

# install.packages("remotes")
remotes::install_github("r-lib/lintr")
library(lintr)

length(all_linters())

#> [1] 113
SaranjeetKaur commented 3 weeks ago

I can't seem to find the contributing guidelines (for the community) in the package directory. Please let me know if I missed something - I was looking for something like the dplyr's CONTRIBUTING.md

IndrajeetPatil commented 3 weeks ago

We have two different guidelines:

The closest thing we have to contributing guidelines is a vignette explaining how to add new linters to the package: https://lintr.r-lib.org/articles/creating_linters.html

IndrajeetPatil commented 3 weeks ago

Do you think this should be explicitly mentioned, with say, As of this writing, the development version of {lintr} from GitHub offers 113 linters? Followed by,

I am not sure. We might have a new release before this submission is accepted and published (cf. https://github.com/r-lib/lintr/issues/2392). WDYT, @MichaelChirico?

SaranjeetKaur commented 3 weeks ago

We have two different guidelines:

The closest thing we have to contributing guidelines is a vignette explaining how to add new linters to the package: https://lintr.r-lib.org/articles/creating_linters.html

Nice! I can see that the vignette on creating_linters talks about submitting a pull request. Do you think it is worthwhile to create a .github/CONTRIBUTING.md file and appropriately link such stuff? It might potentially have sections like:

MichaelChirico commented 3 weeks ago

We might have a new release before this submission is accepted and published

We can push for that if it's a priority, what's the timeline for when this paper would be published?

lrnv commented 3 weeks ago

No timeline, it would be published when it is ready. Yes, usually, people synchronize that with a (at least patch) release, but this is not mandatory. We can also delay publication until this release is made if you think it'll be clearer.

Note that the paper refers to a specific version of the software, so you mifgyht phrase that as "As of versions X.X.X, 113, but more to be expected in the future"

lrnv commented 17 hours ago

@JosiahParry, @SaranjeetKaur gentle bump: what is the status of this review on your side ? Are you still waiting for modifications / improvements ?

SaranjeetKaur commented 17 hours ago

I've dropped some minor comments above. Once they are addressed it should be all good from my side.

IndrajeetPatil commented 17 hours ago

Thanks for the nudge, @lrnv. I have an open PR in the repo to address @SaranjeetKaur's feedback (https://github.com/r-lib/lintr/pull/2683). :)

SaranjeetKaur commented 16 hours ago

Thanks for the prompt response @IndrajeetPatil! The PR looks great!

Do you expect any discussion about the new release or mentioning the specific version of the package in the paper?

Otherwise, this looks all good to me. Thanks a lot for all the amazing work you all have put in!

IndrajeetPatil commented 13 hours ago

@SaranjeetKaur Yes, I would personally prefer to wait for this paper to be approved/published until the next version of this package is out on CRAN.

But I need to hand the baton over to @MichaelChirico at this point as he is the current maintainer.