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.

Sign up for GitHub

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

Jump to bottom

Implement tag based documentation #908

Merged
merged 16 commits into from
Mar 8, 2022
Merged

Implement tag based documentation #908

merged 16 commits into from
Mar 8, 2022

Conversation

AshesITR
Copy link
Collaborator

@AshesITR AshesITR commented Feb 28, 2022 •
edited
Loading

Closes #888

So far:

  • Implemented available_linters(packages = "lintr") to return a tibble of available linters contained in the packages specified
  • Added tests to make sure available_linters(packages = ...) always returns a data frame
  • Added a test to ensure the default tag is consistent with default_linters
  • Described the format of inst/lintr/linters.csv in a Package Authors section of ?available_linters
  • Made each linter have its own help page with a separate Tags section pointing to its tags
  • Added a list of linters to ?linters
  • Created new topics tag_linters for all tags, each listing all linters tagged with the tag, plus a small description
  • Added a list of tags to all linter help pages
  • Added a list of tags to ?linters
  • Update NEWS when changes are final

@AshesITR AshesITR mentioned this pull request Feb 28, 2022
Closed
@AshesITR
Copy link
Collaborator Author

AshesITR commented Feb 28, 2022 •
edited
Loading

A test that might be worth adding would be to check that all exported functions with the _linter suffix appear in linters.csv.
I don't know whether it would be wise to self-test the packages own exports.

This test would pass currently:

expect_setequal(
  grep("_linter$", getNamespaceExports(asNamespace("lintr")), value = TRUE),
  available_linters()[["linter"]]
)

@jimhester
Copy link
Member

We may want to use this PR as an opportunity to switch to markdown style roxygen documentation in lintr. e.g. run usethis::use_roxygen_md()

@AshesITR
Copy link
Collaborator Author

AshesITR commented Mar 1, 2022

We might want to also @seealso the linters help page for all linters and all tags, to improve discoverability.

@MichaelChirico
Copy link
Collaborator

The PR is quite large... it will be easier to review if subdivided. at a glance each of your bullets could be their own PR (though feel free to rearrange).

if we want to avoid merging to main until it's done, let's have a sequence of PRs that we merge into this branch, then to main, WDYT?

@AshesITR
Copy link
Collaborator Author

AshesITR commented Mar 1, 2022

Hmm I'm not so sure this will make it easier to review, since multiple of those points touch nearly every file in the codebase.

If it helps, I can try to write down my refactoring process?

@MichaelChirico
Copy link
Collaborator

I think even so, if I can scroll through all the files looking for just one thing that'll be easier than checking for several things at once

@AshesITR
Copy link
Collaborator Author

AshesITR commented Mar 1, 2022

I see. Will try to make a somewhat sensible sequence of PRs into this branch, force-resetting this one to step 1.

@MichaelChirico
Copy link
Collaborator

thanks! appreciate the extra effort

@AshesITR
Copy link
Collaborator Author

AshesITR commented Mar 3, 2022 •
edited
Loading

@MichaelChirico I've split up the PR so far into 3 separate steps.
You can find Step 1 here, Steps 2 and 3 as AshesITR/lintr#1 and AshesITR/lintr#2 respectively.

I plan on adding a Step 4 to include the suggestion by @jimhester to switch to md based roxygen, as well as to add @seealso linters for a full documentation of available linters to all ?*_linter, ?*_linters and ?available_linters help pages.
WDYT about that wording?

@MichaelChirico
Copy link
Collaborator

@Seealso linters for a full documentation of available linters

how does that translate into the .Rd exactly? \seealso{\code{\link{linters}} for full documentation of available linters}?

@AshesITR
Copy link
Collaborator Author

AshesITR commented Mar 4, 2022

Exactly. See AshesITR/lintr#3.
I opted for a slightly different wording in my first try:

\seealso{
\link{linters} for a complete list of linters available in lintr.
}

@AshesITR
Copy link
Collaborator Author

AshesITR commented Mar 4, 2022

I'm done with Step 4 now.
My suggestion would be to review (comment) in order the PRs
#908, AshesITR/lintr#1, AshesITR/lintr#2, AshesITR/lintr#3.

In case of changes in a step, I would rebase the following PRs on the altered version of the previous step, so in the end we could merge all steps into #908 and finally write up a NEWS bullet and merge #908 into master.

@MichaelChirico
Copy link
Collaborator

OK thanks. I take it that's why this is still marked as draft -- kept as draft until the other PRs are merged here. Starting to review now.

MichaelChirico
MichaelChirico reviewed Mar 4, 2022
View reviewed changes
R/linter_tags.R Outdated Show resolved Hide resolved
MichaelChirico
MichaelChirico reviewed Mar 4, 2022
View reviewed changes
R/linter_tags.R Outdated Show resolved Hide resolved
MichaelChirico
MichaelChirico reviewed Mar 4, 2022
View reviewed changes
R/linter_tags.R Outdated Show resolved Hide resolved
MichaelChirico
MichaelChirico reviewed Mar 4, 2022
View reviewed changes
R/linter_tags.R Outdated Show resolved Hide resolved
MichaelChirico
MichaelChirico reviewed Mar 4, 2022
View reviewed changes
R/linter_tags.R Outdated Show resolved Hide resolved
MichaelChirico
MichaelChirico reviewed Mar 4, 2022
View reviewed changes
R/linter_tags.R Outdated Show resolved Hide resolved
MichaelChirico
MichaelChirico reviewed Mar 4, 2022
View reviewed changes
R/linter_tags.R Outdated Show resolved Hide resolved
MichaelChirico
MichaelChirico reviewed Mar 4, 2022
View reviewed changes
R/linter_tags.R Outdated Show resolved Hide resolved
MichaelChirico
MichaelChirico reviewed Mar 4, 2022
View reviewed changes
R/linter_tags.R Outdated Show resolved Hide resolved
MichaelChirico
MichaelChirico reviewed Mar 4, 2022
View reviewed changes
R/linter_tags.R Outdated Show resolved Hide resolved
@MichaelChirico
Copy link
Collaborator

I'll review the other PR, then we can merge to here. GitHub doesn't make it easy to say "I've reviewed through commit X, show me the diff relative to that" (it's possible but cumbersome)

@MichaelChirico MichaelChirico mentioned this pull request Mar 4, 2022
Closed
@AshesITR
Copy link
Collaborator Author

AshesITR commented Mar 5, 2022

The oldrel-4 build failure is due to a bug in rmarkdown:
rstudio/rmarkdown#2324

@AshesITR AshesITR marked this pull request as ready for review March 7, 2022 07:20
@AshesITR
Copy link
Collaborator Author

AshesITR commented Mar 7, 2022

@MichaelChirico, @jimhester PTAL.
I've added a NEWS bullet with multiple sub-bullets. Not sure if that's the optimal way to present the information since describing this PR in a single bullet seems difficult.

AshesITR
AshesITR commented Mar 7, 2022
View reviewed changes
NEWS.md
Comment on lines +82 to +87
* New tag based documentation pages for linters (#888, @AshesITR)
* Each linter has its own help page
* `?linters` also links to tag help pages, collecting linters with a similar goal
* Each linter can have multiple tags
* New function `available_linters()` to list available linters and their tags
This feature is extensible by package authors providing add-on linters for {lintr}.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we could add the bullets on the top level and duplicate the issue number?

@AshesITR AshesITR requested a review from MichaelChirico March 7, 2022 14:01
MichaelChirico
MichaelChirico reviewed Mar 8, 2022
View reviewed changes
NEWS.md
* `?linters` also links to tag help pages, collecting linters with a similar goal
* Each linter can have multiple tags
* New function `available_linters()` to list available linters and their tags
This feature is extensible by package authors providing add-on linters for {lintr}.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this bullet is pretty vague maybe a See ?available_linters reference?

MichaelChirico
MichaelChirico reviewed Mar 8, 2022
View reviewed changes
NEWS.md
@@ -79,6 +79,12 @@ function calls. (#850, #851, @renkun-ken)
* Debugging functions (`browser()`, `debug()`, `debugcall()`, `debugonce()`, `trace()`, `undebug()`, `untrace()`) are now part of the default set of undesirable functions to help prevent them from being committed by mistake. (#876, @michaelchirico)
* New linter `package_hooks_linter()` runs a series of checks also done by `R CMD check` on the `.onLoad()`, `.onAttach()`, `.Last.lib()` and `.onDetach()` hooks (#882, @MichaelChirico)
* Improved location information for R parse errors (#894, #892, @renkun-ken and @AshesITR)
* New tag based documentation pages for linters (#888, @AshesITR)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would help to motivate this (why as lintr user do I care about this update?). maybe a quick blurb like

Add new tag-based documentation pages for linters to help organize information surrounding the growing number of linters available

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refining:

To help organize information surrounding the growing number of linters available, we've added metadata "tags" for each linter (#888).

bullet: each tag gets a help page, e.g. ?x

WDYT

MichaelChirico
MichaelChirico reviewed Mar 8, 2022
View reviewed changes
NEWS.md
@@ -79,6 +79,12 @@ function calls. (#850, #851, @renkun-ken)
* Debugging functions (`browser()`, `debug()`, `debugcall()`, `debugonce()`, `trace()`, `undebug()`, `untrace()`) are now part of the default set of undesirable functions to help prevent them from being committed by mistake. (#876, @michaelchirico)
* New linter `package_hooks_linter()` runs a series of checks also done by `R CMD check` on the `.onLoad()`, `.onAttach()`, `.Last.lib()` and `.onDetach()` hooks (#882, @MichaelChirico)
* Improved location information for R parse errors (#894, #892, @renkun-ken and @AshesITR)
* New tag based documentation pages for linters (#888, @AshesITR)
* Each linter has its own help page
Copy link
Collaborator

@MichaelChirico MichaelChirico Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe add an example ", e.g. ?seq_linter"

MichaelChirico
MichaelChirico reviewed Mar 8, 2022
View reviewed changes
NEWS.md
@@ -79,6 +79,12 @@ function calls. (#850, #851, @renkun-ken)
* Debugging functions (`browser()`, `debug()`, `debugcall()`, `debugonce()`, `trace()`, `undebug()`, `untrace()`) are now part of the default set of undesirable functions to help prevent them from being committed by mistake. (#876, @michaelchirico)
* New linter `package_hooks_linter()` runs a series of checks also done by `R CMD check` on the `.onLoad()`, `.onAttach()`, `.Last.lib()` and `.onDetach()` hooks (#882, @MichaelChirico)
* Improved location information for R parse errors (#894, #892, @renkun-ken and @AshesITR)
* New tag based documentation pages for linters (#888, @AshesITR)
* Each linter has its own help page
* `?linters` also links to tag help pages, collecting linters with a similar goal
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idea (you may have mentioned this/similar before): it may be nice to have an API to set linters by tag in .lintr, WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An idea might be to export alll those linter lists like default_linters and think of a smart way of deduplication?

e.g. make something like linters: with_defaults(best_practices_linters, readability_linters) work, returning all linters that are tagged any of default, best_practices or readability work?

MichaelChirico
MichaelChirico reviewed Mar 8, 2022
View reviewed changes
Copy link
Collaborator

@MichaelChirico MichaelChirico left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NEWS is the only thing new to this PR right? that's all I've reviewed so far

@AshesITR AshesITR merged commit aafba47 into r-lib:master Mar 8, 2022
@AshesITR
Copy link
Collaborator Author

AshesITR commented Mar 9, 2022

Sorry @MichaelChirico I saw only the last comment. Let's fix these up when preparing for the 3.0.0 release.

@AshesITR AshesITR deleted the feature/tag-based-documentation branch March 19, 2022 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelChirico MichaelChirico MichaelChirico left review comments

@jimhester jimhester jimhester approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Structure of lintr documentation
3 participants
@AshesITR @jimhester @MichaelChirico