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

Bookdown #32

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Bookdown #32

wants to merge 8 commits into from

Conversation

maelle
Copy link
Member

@maelle maelle commented May 27, 2019

tested locally on dev_guide, manually adding typos and removing DESCRIPTION to see whether it still works.

I was a bit stuck because get_wordlist() expected a package so I tweaked as_package().

At lot of the code depends on whether DESCRIPTION is present/absent, not very elegant.

cf #29

@maelle maelle requested a review from jeroen May 27, 2019 13:16
@jeroen
Copy link
Member

jeroen commented May 27, 2019

I don't like that this changes the behavior of as_package to not return an error when the directory is not a package. The purpose is specifically to error when the user runs spell_check_package in a directory that is not a package.

@maelle
Copy link
Member Author

maelle commented May 27, 2019

I don't like that this changes the behavior of as_package to not return an error when the directory is not a package. The purpose is specifically to error when the user runs spell_check_package in a directory that is not a package.

How could I workaround this + still use get_wordlist()?

@maelle
Copy link
Member Author

maelle commented May 27, 2019

Would you be ok with my modifying get_wordlist() instead?

spelling/R/wordlist.R

Lines 53 to 58 in 04ba5cb

get_wordlist <- function(pkg = "."){
pkg <- as_package(pkg)
wordfile <- get_wordfile(pkg$path)
out <- if(file.exists(wordfile))
unlist(strsplit(readLines(wordfile, warn = FALSE, encoding = "UTF-8"), " ", fixed = TRUE))
as.character(out)

Could it e.g. work on a path instead of a package object?

@maelle
Copy link
Member Author

maelle commented May 27, 2019

Btw I'll have the same questions for update_wordlist() that'd be handy for a bookdown too.

new_words <- sort(spell_check_package(pkg$path, vignettes = vignettes, use_wordlist = FALSE)$word)
wordfile <- get_wordfile(pkg)
old_words <- sort(get_wordlist(pkg))
new_words <- sort(spell_check_package(pkg, vignettes = vignettes, use_wordlist = FALSE)$word)
Copy link
Member Author

Choose a reason for hiding this comment

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

how could one make this bookdown-compatible too? another update_wordlist() function?

@codecov-io
Copy link

Codecov Report

Merging #32 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #32   +/-   ##
=======================================
  Coverage   45.36%   45.36%           
=======================================
  Files           7        7           
  Lines         313      313           
=======================================
  Hits          142      142           
  Misses        171      171
Impacted Files Coverage Δ
R/parse-markdown.R 83.33% <ø> (ø) ⬆️
R/check-files.R 65.75% <ø> (ø) ⬆️
R/spell-check.R 30.82% <ø> (ø) ⬆️

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 a6f7bb1...ce08bdb. Read the comment docs.

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.

3 participants