-
-
Notifications
You must be signed in to change notification settings - Fork 39
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
[REVIEW]: lintr: Static Code Analysis for R #7240
Comments
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:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
|
Software report:
Commit count by author:
|
Paper file info: 📄 Wordcount for ✅ The paper includes a |
License info: 🟡 License found: |
@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 |
Review checklist for @JosiahParryConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
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
- Sometimes the users might not be aware
+ Sometimes users might not be aware |
@editorialbot generate pdf |
@JosiahParry Thanks a lot for your feedback! We have updated the draft to address your concerns. P.S. We are also currently looking into a possible encoding issue seen in the PDF output: |
@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:
to the yaml header of the paper. See openjournals/joss#963 for wide discussions if this does not work. |
Hey @SaranjeetKaur have you had time to take a look at this review ? |
Hi @lrnv, |
This is all good from my side! Good work all! Many years in the making :) |
Review checklist for @SaranjeetKaurConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
Thanks for all your work! Great package and documentation! Paper
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. |
Happy to rephrase that way.
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! |
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. |
Installing the development version from GitHub ( remotes::install_github("r-lib/lintr")
library(lintr)
length(all_linters())
#> [1] 113 Whereas installing the stable version on CRAN ( 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 # install.packages("remotes")
remotes::install_github("r-lib/lintr")
library(lintr)
length(all_linters())
#> [1] 113 |
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 |
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: |
I am not sure. We might have a new release before this submission is accepted and published (cf. r-lib/lintr#2392). |
Nice! I can see that the vignette on
|
We can push for that if it's a priority, what's the timeline for when this paper would be published? |
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" |
@JosiahParry, @SaranjeetKaur gentle bump: what is the status of this review on your side ? Are you still waiting for modifications / improvements ? |
I've dropped some minor comments above. Once they are addressed it should be all good from my side. |
Thanks for the nudge, @lrnv. I have an open PR in the repo to address @SaranjeetKaur's feedback (r-lib/lintr#2683). :) |
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! |
@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. |
@IndrajeetPatil - it is all fine by me! I don't have any further comments. Could you or @MichaelChirico, please let us know whenever the next version of the package is out on CRAN? @lrnv - is it okay if we have to wait for this paper to published until the next version of the package is out? |
Is {lintr} release the only current blocker, or are there other pending items? We are also trying to get {data.table} salvaged from the CRAN hammer so I need to prioritize OSS time :) |
Yes its ok, now that the submission is sucessfully reviewed, this can wait. Just bump me in here when you are ready to move forward, and we'll continue the publishing process. |
@MichaelChirico No other pending items from me. Please feel free to prioritise as you see fit and drop a message to @lrnv here whenever you are ready to move to the publication process. |
Submitting author: @jimhester (James Hester)
Repository: https://github.com/r-lib/lintr
Branch with paper.md (empty if default branch):
Version: v3.1.2
Editor: @lrnv
Reviewers: @JosiahParry, @SaranjeetKaur
Archive: Pending
Status
Status badge code:
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:
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
The text was updated successfully, but these errors were encountered: