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

Add support for spell checking roxygen comments #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jimhester
Copy link
Collaborator

roxygen2::parse_file() parses the roxygen comments
in each file. Text from relevant tags is then searched for spelling
errors with hunspell::hunspell() to find misspelled words. Because
roxygen does not store the original positions of parsed tags we then
need to find the misspelled word locations in the original roxygen
comment lines of the source. This is done by find_word_positions().

roxygen2::parse_file() is not in the current CRAN version of roxygen2, but I believe @hadley will be submitting a new version to CRAN in the next week or so.

I used Rcpp mainly for convenience if you would prefer to remove the dependency I can do so.

find_word_positions() returns both the line and the start of the words, this was done to support a later enhancement of having RStudio Markers for misspelled words as suggested in r-lib/devtools#1564. But that will require additional changes in other parts of the code, so I will do that in a separate PR in the future.

`roxygen2::parse_file()` parses the roxygen comments
in each file. Text from relevant tags is then searched for spelling
errors with `hunspell::hunspell()` to find misspelled words. Because
roxygen does not store the original positions of parsed tags we then
need to find the misspelled word locations in the original roxygen
comment lines of the source. This is done by `find_word_positions()`.
@codecov-io
Copy link

codecov-io commented Sep 7, 2017

Codecov Report

Merging #3 into master will decrease coverage by 5.99%.
The diff coverage is 1.69%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master      #3   +/-   ##
======================================
- Coverage    45.2%   39.2%   -6%     
======================================
  Files           6       6           
  Lines         250     278   +28     
======================================
- Hits          113     109    -4     
- Misses        137     169   +32
Impacted Files Coverage Δ
R/spell-check.R 35.77% <0%> (+1.03%) ⬆️
R/check-files.R 42.85% <0%> (-16.08%) ⬇️
src/find_word_positions.cpp 3.84% <3.84%> (ø)
R/language.R
R/remove-chunks.R 88.88% <0%> (+2.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38e0782...46cf627. Read the comment docs.

@hadley
Copy link
Member

hadley commented Sep 7, 2017

roxygen2 should generally be storing the positions of the tags (because they're used for errors)

@jeroen
Copy link
Member

jeroen commented Sep 7, 2017

  • Currently it reports spelling errors both in the roxygen and Rd files. Perhaps that is sensible, but we could also just skip Rd files that start with the % Generated by roxygen2: ... header?
  • Is it really needed to source actual R code? Perhaps we should prefilter only the #' lines? Some packages do funky things or the code can only be sourced after a proper ./configure. E.g. the curl package errors for this reason:
> spell_check_package("~/workspace/curl")
 Show Traceback
 
 Rerun with Debug
 Error in (function() { : 
  Failed to find 'tools/option_table.txt' from:/Users/jeroen/workspace/spelling 

@jimhester
Copy link
Collaborator Author

I think roxygen needs access the objects in general, but perhaps for this use case we might be able to avoid it? I don't think the current API has any way to do that however.

@jeroen
Copy link
Member

jeroen commented Sep 8, 2017

I'm on vacation next week, will review this when im back.

Copy link
Member

@jeroen jeroen left a comment

Choose a reason for hiding this comment

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

Finally time again to look at this. Tested on a few packages, but I'm getting a lot of false positives. Some examples:

  • Parser should ignore inline code chunks. Currently we get lots of false positives for code inside backticks (for markdown-roxygen) or \code{} chunks.
  • Parser should skip \href{} and \url{}
  • Parser should skip @examples and other non-text blocks tags.

Note how the Rd parser uses tools::RdTextFilter which says:

This function blanks out all non-text in an Rd file, for spell checking or other uses.

Ideally the roxygen2 spell checker should behave similarly.

@jimhester
Copy link
Collaborator Author

Do you have an example package where you are seeing this? Pretty sure the code already does the things you mention.

@jeroen
Copy link
Member

jeroen commented Nov 6, 2017

For example spelling the spelling package itself gives:

> spelling::spell_check_package(use_wordlist = FALSE)
  WORD       FOUND IN
CMD        description:3
hunspell   spell_check_files.Rd:17
           spell_check_package.Rd:21
           wordlist.Rd:19
pkg        spell-check.R:21, 22
           spell_check_package.Rd:19
           wordlist.Rd:17
rmd        spell-check.R:5, 22
           spell_check_package.Rd:33
rnw        spell-check.R:5, 22
           spell_check_package.Rd:33

Here the rmd and rnw at spell-check.R:22 are actually inline markdown code. Also pkg at spell-check.R:21 refers to a parameter name, not actual text.

@jimhester
Copy link
Collaborator Author

Ah yes, now I remember. These problems stem from the fact that these terms are misspelled elsewhere in the same roxygen blocks. Because roxygen does not provide accurate line information for parsed blocks (r-lib/roxygen2#664) we don't have the information to find the actual location of the misspelled word. Which is why we have to use find_word_positions() to find the location of the misspelled words in the un-parsed roxygen tags. Because this second search does not used parsed text it does not differentiate between these things, so you will get false positives from words that are misspelled elsewhere in the same roxygen block.

In the cases you cite it is true one of the matches should be ignored, but the other case is normal text. E.g. pkg is a parameter name in line 21, but it is normal text in line 22

#' @param vignettes also check all `rmd` and `rnw` files in the pkg `vignettes` folder

And rmd and rnw are inline code in line 22, but they are again normal text in line 5

#' Parse and spell check R package manual pages, rmd/rnw vignettes, and text fields in the

If we had accurate line information from roxygen this effect would be greatly diminished, as we would only search the exact line for the misspelled word.

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

Successfully merging this pull request may close these issues.

None yet

4 participants