Open jimhester opened 6 years ago
Merging #3 into master will decrease coverage by
5.99%
. The diff coverage is1.69%
.
@@ 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%) |
:arrow_up: |
R/check-files.R | 42.85% <0%> (-16.08%) |
:arrow_down: |
src/find_word_positions.cpp | 3.84% <3.84%> (ø) |
|
R/language.R | ||
R/remove-chunks.R | 88.88% <0%> (+2.52%) |
:arrow_up: |
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.
roxygen2 should generally be storing the positions of the tags (because they're used for errors)
Rd
files that start with the % Generated by roxygen2: ...
header?#'
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
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.
I'm on vacation next week, will review this when im back.
Do you have an example package where you are seeing this? Pretty sure the code already does the things you mention.
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.
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 (https://github.com/klutometis/roxygen/issues/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
https://github.com/ropensci/spelling/blob/65d419d2e356d791be8209cfbb22e194cc5a88b4/R/spell-check.R#L22
And rmd
and rnw
are inline code in line 22, but they are again normal text in line 5
https://github.com/ropensci/spelling/blob/65d419d2e356d791be8209cfbb22e194cc5a88b4/R/spell-check.R#L5
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.
roxygen2::parse_file()
parses the roxygen comments in each file. Text from relevant tags is then searched for spelling errors withhunspell::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 byfind_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 https://github.com/hadley/devtools/issues/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.